Thread: Re: Problem with dblink
Andrea Grassi wrote:
[...using dblink_build_sql_insert() inside a PL/pgSQL function...]
> ERROR: improper call to spi_printtup
> CONTEXT: PL/pgSQL function "f_sync_trigger_a_clifor" line 6 at select into
> variables
Attached fixes a recently reported bug in dblink. If there are no
objections, I'll make this my first commit :-)
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?
Thanks,
Joe
Index: dblink.c
===================================================================
RCS file: /opt/src/cvs/pgsql-server/contrib/dblink/dblink.c,v
retrieving revision 1.27
diff -c -r1.27 dblink.c
*** dblink.c 12 Nov 2003 21:15:42 -0000 1.27
--- dblink.c 21 Nov 2003 18:26:17 -0000
***************
*** 1766,1771 ****
--- 1766,1772 ----
SPITupleTable *tuptable = SPI_tuptable;
tuple = SPI_copytuple(tuptable->vals[0]);
+ SPI_finish();
return tuple;
}
***************
*** 1774,1779 ****
--- 1775,1782 ----
/*
* no qualifying tuples
*/
+ SPI_finish();
+
return NULL;
}
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
Index: src/backend/access/transam/xact.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/access/transam/xact.c,v
retrieving revision 1.157
diff -c -r1.157 xact.c
*** src/backend/access/transam/xact.c 29 Nov 2003 19:51:40 -0000 1.157
--- src/backend/access/transam/xact.c 29 Nov 2003 23:57:09 -0000
***************
*** 977,983 ****
CallEOXactCallbacks(true);
AtEOXact_GUC(true);
! AtEOXact_SPI();
AtEOXact_gist();
AtEOXact_hash();
AtEOXact_nbtree();
--- 977,983 ----
CallEOXactCallbacks(true);
AtEOXact_GUC(true);
! AtEOXact_SPI(false);
AtEOXact_gist();
AtEOXact_hash();
AtEOXact_nbtree();
***************
*** 1087,1093 ****
CallEOXactCallbacks(false);
AtEOXact_GUC(false);
! AtEOXact_SPI();
AtEOXact_gist();
AtEOXact_hash();
AtEOXact_nbtree();
--- 1087,1093 ----
CallEOXactCallbacks(false);
AtEOXact_GUC(false);
! AtEOXact_SPI(true);
AtEOXact_gist();
AtEOXact_hash();
AtEOXact_nbtree();
Index: src/backend/executor/spi.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/executor/spi.c,v
retrieving revision 1.108
diff -c -r1.108 spi.c
*** src/backend/executor/spi.c 29 Nov 2003 19:51:48 -0000 1.108
--- src/backend/executor/spi.c 29 Nov 2003 23:57:09 -0000
***************
*** 177,191 ****
* Clean up SPI state at transaction commit or abort (we don't care which).
*/
void
! AtEOXact_SPI(void)
{
/*
* Note that memory contexts belonging to SPI stack entries will be
* freed automatically, so we can ignore them here. We just need to
* restore our static variables to initial state.
*/
! if (_SPI_stack != NULL) /* there was abort */
free(_SPI_stack);
_SPI_current = _SPI_stack = NULL;
_SPI_connected = _SPI_curid = -1;
SPI_processed = 0;
--- 177,199 ----
* Clean up SPI state at transaction commit or abort (we don't care which).
*/
void
! AtEOXact_SPI(bool isAbort)
{
/*
* Note that memory contexts belonging to SPI stack entries will be
* freed automatically, so we can ignore them here. We just need to
* restore our static variables to initial state.
*/
! 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")));
+ }
+
_SPI_current = _SPI_stack = NULL;
_SPI_connected = _SPI_curid = -1;
SPI_processed = 0;
Index: src/include/executor/spi.h
===================================================================
RCS file: /cvsroot/pgsql-server/src/include/executor/spi.h,v
retrieving revision 1.40
diff -c -r1.40 spi.h
*** src/include/executor/spi.h 29 Nov 2003 22:41:01 -0000 1.40
--- src/include/executor/spi.h 29 Nov 2003 23:57:09 -0000
***************
*** 116,121 ****
extern void SPI_cursor_move(Portal portal, bool forward, int count);
extern void SPI_cursor_close(Portal portal);
! extern void AtEOXact_SPI(void);
#endif /* SPI_H */
--- 116,121 ----
extern void SPI_cursor_move(Portal portal, bool forward, int count);
extern void SPI_cursor_close(Portal portal);
! extern void AtEOXact_SPI(bool isAbort);
#endif /* SPI_H */
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
Index: src/backend/access/transam/xact.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/access/transam/xact.c,v
retrieving revision 1.157
diff -c -r1.157 xact.c
*** src/backend/access/transam/xact.c 29 Nov 2003 19:51:40 -0000 1.157
--- src/backend/access/transam/xact.c 1 Dec 2003 03:50:40 -0000
***************
*** 977,983 ****
CallEOXactCallbacks(true);
AtEOXact_GUC(true);
! AtEOXact_SPI();
AtEOXact_gist();
AtEOXact_hash();
AtEOXact_nbtree();
--- 977,983 ----
CallEOXactCallbacks(true);
AtEOXact_GUC(true);
! AtEOXact_SPI(true);
AtEOXact_gist();
AtEOXact_hash();
AtEOXact_nbtree();
***************
*** 1087,1093 ****
CallEOXactCallbacks(false);
AtEOXact_GUC(false);
! AtEOXact_SPI();
AtEOXact_gist();
AtEOXact_hash();
AtEOXact_nbtree();
--- 1087,1093 ----
CallEOXactCallbacks(false);
AtEOXact_GUC(false);
! AtEOXact_SPI(false);
AtEOXact_gist();
AtEOXact_hash();
AtEOXact_nbtree();
Index: src/backend/executor/spi.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/executor/spi.c,v
retrieving revision 1.108
diff -c -r1.108 spi.c
*** src/backend/executor/spi.c 29 Nov 2003 19:51:48 -0000 1.108
--- src/backend/executor/spi.c 1 Dec 2003 03:50:40 -0000
***************
*** 174,191 ****
}
/*
! * Clean up SPI state at transaction commit or abort (we don't care which).
*/
void
! AtEOXact_SPI(void)
{
/*
* Note that memory contexts belonging to SPI stack entries will be
* freed automatically, so we can ignore them here. We just need to
* restore our static variables to initial state.
*/
! if (_SPI_stack != NULL) /* there was abort */
free(_SPI_stack);
_SPI_current = _SPI_stack = NULL;
_SPI_connected = _SPI_curid = -1;
SPI_processed = 0;
--- 174,199 ----
}
/*
! * Clean up SPI state at transaction commit or abort.
*/
void
! AtEOXact_SPI(bool isCommit)
{
/*
* Note that memory contexts belonging to SPI stack entries will be
* freed automatically, so we can ignore them here. We just need to
* restore our static variables to initial state.
*/
! if (_SPI_stack != NULL)
! {
free(_SPI_stack);
+ if (isCommit)
+ ereport(WARNING,
+ (errcode(ERRCODE_WARNING),
+ errmsg("freeing non-empty SPI stack"),
+ errhint("Check for missing \"SPI_finish\" calls")));
+ }
+
_SPI_current = _SPI_stack = NULL;
_SPI_connected = _SPI_curid = -1;
SPI_processed = 0;
Index: src/include/executor/spi.h
===================================================================
RCS file: /cvsroot/pgsql-server/src/include/executor/spi.h,v
retrieving revision 1.40
diff -c -r1.40 spi.h
*** src/include/executor/spi.h 29 Nov 2003 22:41:01 -0000 1.40
--- src/include/executor/spi.h 1 Dec 2003 03:50:43 -0000
***************
*** 116,121 ****
extern void SPI_cursor_move(Portal portal, bool forward, int count);
extern void SPI_cursor_close(Portal portal);
! extern void AtEOXact_SPI(void);
#endif /* SPI_H */
--- 116,121 ----
extern void SPI_cursor_move(Portal portal, bool forward, int count);
extern void SPI_cursor_close(Portal portal);
! extern void AtEOXact_SPI(bool isCommit);
#endif /* SPI_H */