Skip to content

Commit c3294f1

Browse files
tglsfdctglsfdc
committed
Fix interaction between materializing holdable cursors and firing
deferred triggers: either one can create more work for the other, so we have to loop till it's all gone. Per example from andrew@supernews. Add a regression test to help spot trouble in this area in future.
1 parent 0c400f1 commit c3294f1

File tree

7 files changed

+187
-70
lines changed

7 files changed

+187
-70
lines changed

‎src/backend/access/transam/xact.c

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
*
1111
*
1212
* IDENTIFICATION
13-
* $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.198 2005/03/28 01:50:33 tgl Exp $
13+
* $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.199 2005/04/11 19:51:14 tgl Exp $
1414
*
1515
*-------------------------------------------------------------------------
1616
*/
@@ -1439,16 +1439,32 @@ CommitTransaction(void)
14391439
/*
14401440
* Do pre-commit processing (most of this stuff requires database
14411441
* access, and in fact could still cause an error...)
1442+
*
1443+
* It is possible for CommitHoldablePortals to invoke functions that
1444+
* queue deferred triggers, and it's also possible that triggers create
1445+
* holdable cursors. So we have to loop until there's nothing left to
1446+
* do.
14421447
*/
1448+
for (;;)
1449+
{
1450+
/*
1451+
* Fire all currently pending deferred triggers.
1452+
*/
1453+
AfterTriggerFireDeferred();
14431454

1444-
/*
1445-
* Tell the trigger manager that this transaction is about to be
1446-
* committed. He'll invoke all trigger deferred until XACT before we
1447-
* really start on committing the transaction.
1448-
*/
1449-
AfterTriggerEndXact();
1455+
/*
1456+
* Convert any open holdable cursors into static portals. If there
1457+
* weren't any, we are done ... otherwise loop back to check if they
1458+
* queued deferred triggers. Lather, rinse, repeat.
1459+
*/
1460+
if (!CommitHoldablePortals())
1461+
break;
1462+
}
1463+
1464+
/* Now we can shut down the deferred-trigger manager */
1465+
AfterTriggerEndXact(true);
14501466

1451-
/* Close open cursors */
1467+
/* Close any open regular cursors */
14521468
AtCommit_Portals();
14531469

14541470
/*
@@ -1649,7 +1665,7 @@ AbortTransaction(void)
16491665
/*
16501666
* do abort processing
16511667
*/
1652-
AfterTriggerAbortXact();
1668+
AfterTriggerEndXact(false);
16531669
AtAbort_Portals();
16541670
AtEOXact_LargeObject(false); /* 'false' means it's abort */
16551671
AtAbort_Notify();

‎src/backend/commands/trigger.c

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1994, Regents of the University of California
88
*
99
* IDENTIFICATION
10-
* $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.183 2005/03/29 03:01:30 tgl Exp $
10+
* $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.184 2005/04/11 19:51:15 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -2441,14 +2441,18 @@ AfterTriggerEndQuery(EState *estate)
24412441

24422442

24432443
/* ----------
2444-
* AfterTriggerEndXact()
2444+
* AfterTriggerFireDeferred()
24452445
*
24462446
* Called just before the current transaction is committed. At this
2447-
* time we invoke all DEFERRED triggers and tidy up.
2447+
* time we invoke all pending DEFERRED triggers.
2448+
*
2449+
* It is possible for other modules to queue additional deferred triggers
2450+
* during pre-commit processing; therefore xact.c may have to call this
2451+
* multiple times.
24482452
* ----------
24492453
*/
24502454
void
2451-
AfterTriggerEndXact(void)
2455+
AfterTriggerFireDeferred(void)
24522456
{
24532457
AfterTriggerEventList *events;
24542458

@@ -2463,49 +2467,41 @@ AfterTriggerEndXact(void)
24632467
* for them to use. (Since PortalRunUtility doesn't set a snap for
24642468
* COMMIT, we can't assume ActiveSnapshot is valid on entry.)
24652469
*/
2466-
if (afterTriggers->events.head != NULL)
2470+
events = &afterTriggers->events;
2471+
if (events->head != NULL)
24672472
ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
24682473

24692474
/*
24702475
* Run all the remaining triggers. Loop until they are all gone,
24712476
* just in case some trigger queues more for us to do.
24722477
*/
2473-
events = &afterTriggers->events;
24742478
while (afterTriggerMarkEvents(events, NULL, false))
24752479
{
24762480
CommandId firing_id = afterTriggers->firing_counter++;
24772481

24782482
afterTriggerInvokeEvents(events, firing_id, NULL, true);
24792483
}
24802484

2481-
/*
2482-
* Forget everything we know about AFTER triggers.
2483-
*
2484-
* Since all the info is in TopTransactionContext or children thereof, we
2485-
* need do nothing special to reclaim memory.
2486-
*/
2487-
afterTriggers = NULL;
2485+
Assert(events->head == NULL);
24882486
}
24892487

24902488

24912489
/* ----------
2492-
* AfterTriggerAbortXact()
2490+
* AfterTriggerEndXact()
2491+
*
2492+
* The current transaction is finishing.
24932493
*
2494-
* The current transaction has entered the abort state.
2495-
* All outstanding triggers are canceled so we simply throw
2494+
* Any unfired triggers are canceled so we simply throw
24962495
* away anything we know.
2496+
*
2497+
* Note: it is possible for this to be called repeatedly in case of
2498+
* error during transaction abort; therefore, do not complain if
2499+
* already closed down.
24972500
* ----------
24982501
*/
24992502
void
2500-
AfterTriggerAbortXact(void)
2503+
AfterTriggerEndXact(bool isCommit)
25012504
{
2502-
/*
2503-
* Ignore call if we aren't in a transaction. (Need this to survive
2504-
* repeat call in case of error during transaction abort.)
2505-
*/
2506-
if (afterTriggers == NULL)
2507-
return;
2508-
25092505
/*
25102506
* Forget everything we know about AFTER triggers.
25112507
*

‎src/backend/utils/mmgr/portalmem.c

Lines changed: 59 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
* Portions Copyright (c) 1994, Regents of the University of California
1313
*
1414
* IDENTIFICATION
15-
* $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.77 2005/01/26 23:20:21 tgl Exp $
15+
* $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.78 2005/04/11 19:51:15 tgl Exp $
1616
*
1717
*-------------------------------------------------------------------------
1818
*/
@@ -416,18 +416,14 @@ DropDependentPortals(MemoryContext queryContext)
416416
*
417417
* Any holdable cursors created in this transaction need to be converted to
418418
* materialized form, since we are going to close down the executor and
419-
* release locks. Remove all other portals created in this transaction.
420-
* Portals remaining from prior transactions should be left untouched.
419+
* release locks. Other portals are not touched yet.
421420
*
422-
* XXX This assumes that portals can be deleted in a random order, ie,
423-
* no portal has a reference to any other (at least not one that will be
424-
* exercised during deletion). I think this is okay at the moment, but
425-
* we've had bugs of that ilk in the past. Keep a close eye on cursor
426-
* references...
421+
* Returns TRUE if any holdable cursors were processed, FALSE if not.
427422
*/
428-
void
429-
AtCommit_Portals(void)
423+
bool
424+
CommitHoldablePortals(void)
430425
{
426+
bool result = false;
431427
HASH_SEQ_STATUS status;
432428
PortalHashEnt *hentry;
433429

@@ -437,27 +433,9 @@ AtCommit_Portals(void)
437433
{
438434
Portal portal = hentry->portal;
439435

440-
/*
441-
* Do not touch active portals --- this can only happen in the
442-
* case of a multi-transaction utility command, such as VACUUM.
443-
*
444-
* Note however that any resource owner attached to such a portal is
445-
* still going to go away, so don't leave a dangling pointer.
446-
*/
447-
if (portal->status == PORTAL_ACTIVE)
448-
{
449-
portal->resowner = NULL;
450-
continue;
451-
}
452-
453-
/*
454-
* Do nothing else to cursors held over from a previous
455-
* transaction.
456-
*/
457-
if (portal->createSubid == InvalidSubTransactionId)
458-
continue;
459-
436+
/* Is it a holdable portal created in the current xact? */
460437
if ((portal->cursorOptions & CURSOR_OPT_HOLD) &&
438+
portal->createSubid != InvalidSubTransactionId &&
461439
portal->status == PORTAL_READY)
462440
{
463441
/*
@@ -484,12 +462,60 @@ AtCommit_Portals(void)
484462
* as not belonging to this transaction.
485463
*/
486464
portal->createSubid = InvalidSubTransactionId;
465+
466+
result = true;
487467
}
488-
else
468+
}
469+
470+
return result;
471+
}
472+
473+
/*
474+
* Pre-commit processing for portals.
475+
*
476+
* Remove all non-holdable portals created in this transaction.
477+
* Portals remaining from prior transactions should be left untouched.
478+
*
479+
* XXX This assumes that portals can be deleted in a random order, ie,
480+
* no portal has a reference to any other (at least not one that will be
481+
* exercised during deletion). I think this is okay at the moment, but
482+
* we've had bugs of that ilk in the past. Keep a close eye on cursor
483+
* references...
484+
*/
485+
void
486+
AtCommit_Portals(void)
487+
{
488+
HASH_SEQ_STATUS status;
489+
PortalHashEnt *hentry;
490+
491+
hash_seq_init(&status, PortalHashTable);
492+
493+
while ((hentry = (PortalHashEnt *) hash_seq_search(&status)) != NULL)
494+
{
495+
Portal portal = hentry->portal;
496+
497+
/*
498+
* Do not touch active portals --- this can only happen in the
499+
* case of a multi-transaction utility command, such as VACUUM.
500+
*
501+
* Note however that any resource owner attached to such a portal is
502+
* still going to go away, so don't leave a dangling pointer.
503+
*/
504+
if (portal->status == PORTAL_ACTIVE)
489505
{
490-
/* Zap all non-holdable portals */
491-
PortalDrop(portal, true);
506+
portal->resowner = NULL;
507+
continue;
492508
}
509+
510+
/*
511+
* Do nothing to cursors held over from a previous transaction
512+
* (including holdable ones just frozen by CommitHoldablePortals).
513+
*/
514+
if (portal->createSubid == InvalidSubTransactionId)
515+
continue;
516+
517+
/* Zap all non-holdable portals */
518+
PortalDrop(portal, true);
493519
}
494520
}
495521

‎src/include/commands/trigger.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
77
* Portions Copyright (c) 1994, Regents of the University of California
88
*
9-
* $PostgreSQL: pgsql/src/include/commands/trigger.h,v 1.52 2005/03/25 21:57:59 tgl Exp $
9+
* $PostgreSQL: pgsql/src/include/commands/trigger.h,v 1.53 2005/04/11 19:51:15 tgl Exp $
1010
*
1111
*-------------------------------------------------------------------------
1212
*/
@@ -157,8 +157,8 @@ extern void ExecARUpdateTriggers(EState *estate,
157157
extern void AfterTriggerBeginXact(void);
158158
extern void AfterTriggerBeginQuery(void);
159159
extern void AfterTriggerEndQuery(EState *estate);
160-
extern void AfterTriggerEndXact(void);
161-
extern void AfterTriggerAbortXact(void);
160+
extern void AfterTriggerFireDeferred(void);
161+
extern void AfterTriggerEndXact(bool isCommit);
162162
extern void AfterTriggerBeginSubXact(void);
163163
extern void AfterTriggerEndSubXact(bool isCommit);
164164

‎src/include/utils/portal.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@
3939
* Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
4040
* Portions Copyright (c) 1994, Regents of the University of California
4141
*
42-
* $PostgreSQL: pgsql/src/include/utils/portal.h,v 1.54 2004/12/31 22:03:46 pgsql Exp $
42+
* $PostgreSQL: pgsql/src/include/utils/portal.h,v 1.55 2005/04/11 19:51:16 tgl Exp $
4343
*
4444
*-------------------------------------------------------------------------
4545
*/
@@ -182,6 +182,7 @@ typedef struct PortalData
182182

183183
/* Prototypes for functions in utils/mmgr/portalmem.c */
184184
extern void EnablePortalManager(void);
185+
extern bool CommitHoldablePortals(void);
185186
extern void AtCommit_Portals(void);
186187
extern void AtAbort_Portals(void);
187188
extern void AtCleanup_Portals(void);

‎src/test/regress/expected/portals.out

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -773,3 +773,38 @@ FETCH ALL FROM c;
773773
(15 rows)
774774

775775
ROLLBACK;
776+
--
777+
-- Test behavior of both volatile and stable functions inside a cursor;
778+
-- in particular we want to see what happens during commit of a holdable
779+
-- cursor
780+
--
781+
create temp table tt1(f1 int);
782+
create function count_tt1_v() returns int8 as
783+
'select count(*) from tt1' language sql volatile;
784+
create function count_tt1_s() returns int8 as
785+
'select count(*) from tt1' language sql stable;
786+
begin;
787+
insert into tt1 values(1);
788+
declare c1 cursor for select count_tt1_v(), count_tt1_s();
789+
insert into tt1 values(2);
790+
fetch all from c1;
791+
count_tt1_v | count_tt1_s
792+
-------------+-------------
793+
2 | 1
794+
(1 row)
795+
796+
rollback;
797+
begin;
798+
insert into tt1 values(1);
799+
declare c2 cursor with hold for select count_tt1_v(), count_tt1_s();
800+
insert into tt1 values(2);
801+
commit;
802+
delete from tt1;
803+
fetch all from c2;
804+
count_tt1_v | count_tt1_s
805+
-------------+-------------
806+
2 | 1
807+
(1 row)
808+
809+
drop function count_tt1_v();
810+
drop function count_tt1_s();

0 commit comments

Comments
 (0)