Re: Problem with dblink

Joe Conway <mail@joeconway.com> writes:

Attached fixes a recently reported bug in dblink. If there are no
objections, I'll make this my first commit :-)

Looks right to me; but as a general tip, if you've made mistake X in
place A, you might have made it in place B too. Have you checked the
rest of dblink for forgotten SPI_finish calls?

(More generally, I wonder if AtEOXact_SPI oughtn't be fixed to emit
a WARNING if the SPI stack isn't empty, except in the error case.
Neglecting SPI_finish is analogous to a resource leak, and we have
code in place to warn about other sorts of leaks.)

I intend to apply it to HEAD and REL7_4_STABLE, but it could also be
applied to REL7_3_STABLE -- any opinions on that? Are we still
considering a 7.3.5 release?

Sure. I believe we still intend a 7.3.5 shortly (maybe early December,
since Thanksgiving week is a bad time to expect anything to happen).

regards, tom lane

Tom Lane wrote:

Looks right to me; but as a general tip, if you've made mistake X in
place A, you might have made it in place B too. Have you checked the
rest of dblink for forgotten SPI_finish calls?

Good tip -- will do. Though, I suspect the dblink_build_sql_* functions
have been much more lightly used than the rest of dblink, and that's the
only reason this bug has lurked for so long.

(More generally, I wonder if AtEOXact_SPI oughtn't be fixed to emit
a WARNING if the SPI stack isn't empty, except in the error case.
Neglecting SPI_finish is analogous to a resource leak, and we have
code in place to warn about other sorts of leaks.)

I'll take a look at this too.

Sure. I believe we still intend a 7.3.5 shortly (maybe early December,
since Thanksgiving week is a bad time to expect anything to happen).

OK -- then I'll plan to apply the final fix to REL7_3_STABLE as well.

Thanks,

Joe

Tom Lane wrote:

Looks right to me; but as a general tip, if you've made mistake X in
place A, you might have made it in place B too. Have you checked the
rest of dblink for forgotten SPI_finish calls?

That function was the only one using SPI calls in dblink, so there were
no others. Fix applied to head, 7.3 stable, and 7.4 stable.

(More generally, I wonder if AtEOXact_SPI oughtn't be fixed to emit
a WARNING if the SPI stack isn't empty, except in the error case.
Neglecting SPI_finish is analogous to a resource leak, and we have
code in place to warn about other sorts of leaks.)

I'll look at this separately. Changes in this regard only belong applied
to cvs head I assume.

Joe

Joe Conway <mail@joeconway.com> writes:

Tom Lane wrote:

(More generally, I wonder if AtEOXact_SPI oughtn't be fixed to emit
a WARNING if the SPI stack isn't empty, except in the error case.
Neglecting SPI_finish is analogous to a resource leak, and we have
code in place to warn about other sorts of leaks.)

I'll look at this separately. Changes in this regard only belong applied
to cvs head I assume.

Yeah, I think to apply it to a stable branch you'd want some indication
that there are more live bugs out there that it's needed to catch. At
the moment it only seems called for as an aid to future development, and
so HEAD seems sufficient.

regards, tom lane

Tom Lane wrote:

Yeah, I think to apply it to a stable branch you'd want some indication
that there are more live bugs out there that it's needed to catch. At
the moment it only seems called for as an aid to future development, and
so HEAD seems sufficient.

Thanks, that's what i thought.

On a loosely related question, I didn't see commit messages for my
commits. Am I supposed to do something specific to cause them to be emitted?

Joe

Thanks, that's what i thought.

On a loosely related question, I didn't see commit messages for my
commits. Am I supposed to do something specific to cause them to be
emitted?

I saw them, "User Joe" :)

CVSROOT: /cvsroot
Module name: pgsql-server
Changes by: joe@svr1.postgresql.org 03/11/26 16:43:25

Modified files:
contrib/dblink : dblink.c

Log message:
Added missing SPI_finish() calls to get_tuple_of_interest(). Fixes bug
reported by Andrea Grassi.

...and the rest...

Chris

Christopher Kings-Lynne wrote:

I saw them, "User Joe" :)

Yeah, they just showed up for me too. I'll have to figure out how to
change that from "User Joe" to something else -- sounds kind of strange ;-)

Joe

Joe Conway wrote:

Christopher Kings-Lynne wrote:

I saw them, "User Joe" :)

Yeah, they just showed up for me too. I'll have to figure out how to
change that from "User Joe" to something else -- sounds kind of strange ;-)

Marc just fixed it.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Wed, 26 Nov 2003, Joe Conway wrote:

Tom Lane wrote:

Yeah, I think to apply it to a stable branch you'd want some indication
that there are more live bugs out there that it's needed to catch. At
the moment it only seems called for as an aid to future development, and
so HEAD seems sufficient.

Thanks, that's what i thought.

On a loosely related question, I didn't see commit messages for my
commits. Am I supposed to do something specific to cause them to be emitted?

I did see them (although you ought to get Marc to change the Real Name
field to show Joe Conway instead of User joe).

LER

Joe

---------------------------(end of broadcast)---------------------------
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to majordomo@postgresql.org)

--
Larry Rosenman http://www.lerctr.org/~ler
Phone: +1 972-414-9812 E-Mail: ler@lerctr.org
US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749

On Wed, 26 Nov 2003, Joe Conway wrote:

Christopher Kings-Lynne wrote:

I saw them, "User Joe" :)

Yeah, they just showed up for me too. I'll have to figure out how to
change that from "User Joe" to something else -- sounds kind of strange

;-)
Does Marc allow chfn to run on those boxes?

If so, run it. :-)

LER

Joe

---------------------------(end of broadcast)---------------------------
TIP 8: explain analyze is your friend

--
Larry Rosenman http://www.lerctr.org/~ler
Phone: +1 972-414-9812 E-Mail: ler@lerctr.org
US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749

Bruce Momjian wrote:

Joe Conway wrote:

Yeah, they just showed up for me too. I'll have to figure out how to
change that from "User Joe" to something else -- sounds kind of strange ;-)

Marc just fixed it.

Ah, great! Thanks Marc :-)

Joe

Tom Lane wrote:

(More generally, I wonder if AtEOXact_SPI oughtn't be fixed to emit
a WARNING if the SPI stack isn't empty, except in the error case.
Neglecting SPI_finish is analogous to a resource leak, and we have
code in place to warn about other sorts of leaks.)

Is the attached what you had in mind? The original problem function
would now look like:

SELECT dblink_build_sql_insert('foo','1 2',2,'{"0", "a"}','{"99", "xyz"}');
WARNING: freeing non-empty SPI stack
HINT: Check for missing "SPI_finish" calls
dblink_build_sql_insert
-----------------------------------------------------------
INSERT INTO foo(f1,f2,f3) VALUES('99','xyz','{a0,b0,c0}')
(1 row)

Joe

Joe Conway <mail@joeconway.com> writes:

Tom Lane wrote:
(More generally, I wonder if AtEOXact_SPI oughtn't be fixed to emit
a WARNING if the SPI stack isn't empty, except in the error case.
Neglecting SPI_finish is analogous to a resource leak, and we have
code in place to warn about other sorts of leaks.)

Is the attached what you had in mind?

Approximately. A couple minor stylistic gripes:

1. AFAIR, all the other at-end-of-xact routines that take a flag telling
them if it's commit or abort define the flag as isCommit. Having the
reverse convention for this one routine is confusing and a recipe for
errors, so please make it be isCommit too.

2. The initial comment for AtEOXact_SPI:

* Clean up SPI state at transaction commit or abort (we don't care which).

is now incorrect and needs to be updated (at least get rid of the
parenthetical note).

! 	if (_SPI_stack != NULL)
! 	{
free(_SPI_stack);
+ 		if (!isAbort)
+ 			ereport(WARNING,
+ 					(errcode(ERRCODE_WARNING),
+ 					 errmsg("freeing non-empty SPI stack"),
+ 					 errhint("Check for missing \"SPI_finish\" calls")));
+ 	}
+ 

While this isn't necessarily wrong, what would probably be a stronger
test is to complain if either _SPI_connected or _SPI_curid is not -1.
For one thing that would catch missing SPI_pop(). (Jan, any comment
about that?)

regards, tom lane

Tom Lane wrote:

1. AFAIR, all the other at-end-of-xact routines that take a flag telling
them if it's commit or abort define the flag as isCommit. Having the
reverse convention for this one routine is confusing and a recipe for
errors, so please make it be isCommit too.

Done.

2. The initial comment for AtEOXact_SPI:

* Clean up SPI state at transaction commit or abort (we don't care which).

is now incorrect and needs to be updated (at least get rid of the
parenthetical note).

Done.

! 	if (_SPI_stack != NULL)
! 	{
free(_SPI_stack);
+ 		if (!isAbort)
+ 			ereport(WARNING,
+ 					(errcode(ERRCODE_WARNING),
+ 					 errmsg("freeing non-empty SPI stack"),
+ 					 errhint("Check for missing \"SPI_finish\" calls")));
+ 	}
+ 

While this isn't necessarily wrong, what would probably be a stronger
test is to complain if either _SPI_connected or _SPI_curid is not -1.
For one thing that would catch missing SPI_pop(). (Jan, any comment
about that?)

Based on some simple testing, a warning for (_SPI_connected != -1) seems
redundant with (_SPI_stack != NULL) -- i.e. I just get both warnings
when SPI_finish() isn't called. And commenting out SPI_pop() causes a
"SPI_finish failed" error, at least in the one place it's used
(pl_exec.c), so we never get to AtEOXact_SPI(). So for the moment at
least I left that last section the same.

OK to commit?

Joe