Thread: BUG #1592: "with hold" cursor problem

The following bug has been logged online:

Bug reference:      1592
Logged by:          Gabor Berenyi
Email address:      ber_@freemail.hu
PostgreSQL version: 8.0.0
Operating system:   i86 Debian Linux 2.4.20
Description:        "with hold" cursor problem
Details:

begin;
declare t cursor with hold for select function() as x from R;
commit;

Documentation says that the rows represented by a held cursor are copied
into a temporary file or memory area, but they are not, if they are results
of a function call. Instead, "commit" aborts the connection.

"Gabor Berenyi" <ber_@freemail.hu> writes:
> begin;
> declare t cursor with hold for select function() as x from R;
> commit;

> Documentation says that the rows represented by a held cursor are copied
> into a temporary file or memory area, but they are not, if they are results
> of a function call. Instead, "commit" aborts the connection.

Works for me.  Could we see a complete, self-contained example?
In particular I suppose that the details of function() might be
relevant.

            regards, tom lane

On 2005-04-10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Gabor Berenyi" <ber_@freemail.hu> writes:
>> begin;
>> declare t cursor with hold for select function() as x from R;
>> commit;
>
>> Documentation says that the rows represented by a held cursor are copied
>> into a temporary file or memory area, but they are not, if they are results
>> of a function call. Instead, "commit" aborts the connection.
>
> Works for me.  Could we see a complete, self-contained example?
> In particular I suppose that the details of function() might be
> relevant.

Someone mentioned this on IRC; I was going to post an analysis but I got
sidetracked with other stuff. The critical detail is that function()
has to be an SQL function declared stable (or possibly immutable, not
sure) and it must invoke a volatile function such as timeofday(). The
crash happens in CopySnapshot trying to copy a null snapshot.

Obviously the function definition is in itself illegal, so the workaround
is "don't do that", but the backend should probably do something more
useful than segfaulting.

Complete example:

test=# create function crashme() returns text as 'select timeofday()'
test-#   language sql stable;
CREATE FUNCTION
test=# begin;
BEGIN
test=# declare t cursor with hold for select crashme() as x from pg_class;
DECLARE CURSOR
test=# commit;
server closed the connection unexpectedly

--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services

Andrew - Supernews <andrew+nonews@supernews.com> writes:
> test=# create function crashme() returns text as 'select timeofday()'
> test-#   language sql stable;
> CREATE FUNCTION
> test=# begin;
> BEGIN
> test=# declare t cursor with hold for select crashme() as x from pg_class;
> DECLARE CURSOR
> test=# commit;
> server closed the connection unexpectedly

You don't even need the begin/commit ...

The immediate problem is that PersistHoldablePortal() forgot to
establish the portal's snapshot as ActiveSnapshot while running the
executor to collect the rest of the cursor data.  This is trivial
to fix, but after fixing that, it crashes elsewhere:

#4  0x2cf01c in ExceptionalCondition (
    conditionName=0xa2998 "!(afterTriggers != ((void *)0))",
    errorType=0xa21dc "FailedAssertion", fileName=0xa1fa4 "trigger.c",
    lineNumber=2370) at assert.c:51
#5  0x18fe9c in AfterTriggerBeginQuery () at trigger.c:2370
#6  0x1aee04 in postquel_start (es=0x4010f310, fcache=0x4010ed28)
    at functions.c:332
#7  0x1af168 in postquel_execute (es=0x7b02fed0, fcinfo=0x7f,
    fcache=0x7b02fec8, resultcontext=0x4006b0d8) at functions.c:466
#8  0x1af3b8 in fmgr_sql (fcinfo=0x7b03c1c0) at functions.c:639
...
#17 0x1a3994 in ExecutorRun (queryDesc=0x4010d188,
    direction=ForwardScanDirection, count=0) at execMain.c:228
#18 0x17b0a0 in PersistHoldablePortal (portal=0x400c40a0) at portalcmds.c:379
#19 0x2eb968 in AtCommit_Portals () at portalmem.c:473
#20 0x10dc5c in CommitTransaction () at xact.c:1452
#21 0x10e37c in CommitTransactionCommand () at xact.c:1941
#22 0x2480d4 in finish_xact_command () at postgres.c:1844

The problem here is that CommitTransaction shuts down the trigger
manager before shutting down portals, so of course trigger.c barfs
when the SQL function wants it to check for queued triggers.

There seems to be a rather fundamental ordering problem here,
since it's certainly possible for a holdable portal to contain
volatile functions that must fire triggers, and yet I suppose that
a trigger might also want to use or create cursors.  Perhaps we
have to iterate "close holdable cursors" and "fire triggers" until
there's nothing more to do.  Thoughts anyone?

            regards, tom lane

On Mon, Apr 11, 2005 at 11:49:22AM -0400, Tom Lane wrote:

> The problem here is that CommitTransaction shuts down the trigger
> manager before shutting down portals, so of course trigger.c barfs
> when the SQL function wants it to check for queued triggers.
>
> There seems to be a rather fundamental ordering problem here,
> since it's certainly possible for a holdable portal to contain
> volatile functions that must fire triggers, and yet I suppose that
> a trigger might also want to use or create cursors.  Perhaps we
> have to iterate "close holdable cursors" and "fire triggers" until
> there's nothing more to do.  Thoughts anyone?

One further question is if it's right for the holdable portal to fire
trigger at transaction commit.  ISTM the non-surprising answer would be
for them to fire at FETCH time.  (But of course this is hard to
implement, since at that time we don't have executor machinery ...)

Barring that, yes, maybe the best answer is to iterate between those,
and shut down the trigger manager only after everything's done.

--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"This is a foot just waiting to be shot"                (Andrew Dunstan)

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> One further question is if it's right for the holdable portal to fire
> trigger at transaction commit.  ISTM the non-surprising answer would be
> for them to fire at FETCH time.  (But of course this is hard to
> implement, since at that time we don't have executor machinery ...)

Our definition of a holdable cursor is that the query execution is
completed during transaction commit, and all you have left after that
is a static image of the results.  So it's consistent to fire any
associated triggers during commit, too.

            regards, tom lane