From c3294f1cbfe02293b4a7c6b2e58ca4c09a7e541f Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 11 Apr 2005 19:51:16 +0000 Subject: [PATCH] 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. --- src/backend/access/transam/xact.c | 34 +++++++++---- src/backend/commands/trigger.c | 44 ++++++++--------- src/backend/utils/mmgr/portalmem.c | 92 ++++++++++++++++++++++------------- src/include/commands/trigger.h | 6 +-- src/include/utils/portal.h | 3 +- src/test/regress/expected/portals.out | 35 +++++++++++++ src/test/regress/sql/portals.sql | 43 ++++++++++++++++ 7 files changed, 187 insertions(+), 70 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index b5bb3d5b1f..4d896ef551 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -10,7 +10,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.198 2005/03/28 01:50:33 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.199 2005/04/11 19:51:14 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -1439,16 +1439,32 @@ CommitTransaction(void) /* * Do pre-commit processing (most of this stuff requires database * access, and in fact could still cause an error...) + * + * It is possible for CommitHoldablePortals to invoke functions that + * queue deferred triggers, and it's also possible that triggers create + * holdable cursors. So we have to loop until there's nothing left to + * do. */ + for (;;) + { + /* + * Fire all currently pending deferred triggers. + */ + AfterTriggerFireDeferred(); - /* - * Tell the trigger manager that this transaction is about to be - * committed. He'll invoke all trigger deferred until XACT before we - * really start on committing the transaction. - */ - AfterTriggerEndXact(); + /* + * Convert any open holdable cursors into static portals. If there + * weren't any, we are done ... otherwise loop back to check if they + * queued deferred triggers. Lather, rinse, repeat. + */ + if (!CommitHoldablePortals()) + break; + } + + /* Now we can shut down the deferred-trigger manager */ + AfterTriggerEndXact(true); - /* Close open cursors */ + /* Close any open regular cursors */ AtCommit_Portals(); /* @@ -1649,7 +1665,7 @@ AbortTransaction(void) /* * do abort processing */ - AfterTriggerAbortXact(); + AfterTriggerEndXact(false); AtAbort_Portals(); AtEOXact_LargeObject(false); /* 'false' means it's abort */ AtAbort_Notify(); diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index ef00943055..8c8ed9e21c 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -7,7 +7,7 @@ * Portions Copyright (c) 1994, Regents of the University of California * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.183 2005/03/29 03:01:30 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.184 2005/04/11 19:51:15 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -2441,14 +2441,18 @@ AfterTriggerEndQuery(EState *estate) /* ---------- - * AfterTriggerEndXact() + * AfterTriggerFireDeferred() * * Called just before the current transaction is committed. At this - * time we invoke all DEFERRED triggers and tidy up. + * time we invoke all pending DEFERRED triggers. + * + * It is possible for other modules to queue additional deferred triggers + * during pre-commit processing; therefore xact.c may have to call this + * multiple times. * ---------- */ void -AfterTriggerEndXact(void) +AfterTriggerFireDeferred(void) { AfterTriggerEventList *events; @@ -2463,14 +2467,14 @@ AfterTriggerEndXact(void) * for them to use. (Since PortalRunUtility doesn't set a snap for * COMMIT, we can't assume ActiveSnapshot is valid on entry.) */ - if (afterTriggers->events.head != NULL) + events = &afterTriggers->events; + if (events->head != NULL) ActiveSnapshot = CopySnapshot(GetTransactionSnapshot()); /* * Run all the remaining triggers. Loop until they are all gone, * just in case some trigger queues more for us to do. */ - events = &afterTriggers->events; while (afterTriggerMarkEvents(events, NULL, false)) { CommandId firing_id = afterTriggers->firing_counter++; @@ -2478,35 +2482,27 @@ AfterTriggerEndXact(void) afterTriggerInvokeEvents(events, firing_id, NULL, true); } - /* - * Forget everything we know about AFTER triggers. - * - * Since all the info is in TopTransactionContext or children thereof, we - * need do nothing special to reclaim memory. - */ - afterTriggers = NULL; + Assert(events->head == NULL); } /* ---------- - * AfterTriggerAbortXact() + * AfterTriggerEndXact() + * + * The current transaction is finishing. * - * The current transaction has entered the abort state. - * All outstanding triggers are canceled so we simply throw + * Any unfired triggers are canceled so we simply throw * away anything we know. + * + * Note: it is possible for this to be called repeatedly in case of + * error during transaction abort; therefore, do not complain if + * already closed down. * ---------- */ void -AfterTriggerAbortXact(void) +AfterTriggerEndXact(bool isCommit) { /* - * Ignore call if we aren't in a transaction. (Need this to survive - * repeat call in case of error during transaction abort.) - */ - if (afterTriggers == NULL) - return; - - /* * Forget everything we know about AFTER triggers. * * Since all the info is in TopTransactionContext or children thereof, we diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c index 95e1972fe3..fd8a737ada 100644 --- a/src/backend/utils/mmgr/portalmem.c +++ b/src/backend/utils/mmgr/portalmem.c @@ -12,7 +12,7 @@ * Portions Copyright (c) 1994, Regents of the University of California * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.77 2005/01/26 23:20:21 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.78 2005/04/11 19:51:15 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -416,18 +416,14 @@ DropDependentPortals(MemoryContext queryContext) * * Any holdable cursors created in this transaction need to be converted to * materialized form, since we are going to close down the executor and - * release locks. Remove all other portals created in this transaction. - * Portals remaining from prior transactions should be left untouched. + * release locks. Other portals are not touched yet. * - * XXX This assumes that portals can be deleted in a random order, ie, - * no portal has a reference to any other (at least not one that will be - * exercised during deletion). I think this is okay at the moment, but - * we've had bugs of that ilk in the past. Keep a close eye on cursor - * references... + * Returns TRUE if any holdable cursors were processed, FALSE if not. */ -void -AtCommit_Portals(void) +bool +CommitHoldablePortals(void) { + bool result = false; HASH_SEQ_STATUS status; PortalHashEnt *hentry; @@ -437,27 +433,9 @@ AtCommit_Portals(void) { Portal portal = hentry->portal; - /* - * Do not touch active portals --- this can only happen in the - * case of a multi-transaction utility command, such as VACUUM. - * - * Note however that any resource owner attached to such a portal is - * still going to go away, so don't leave a dangling pointer. - */ - if (portal->status == PORTAL_ACTIVE) - { - portal->resowner = NULL; - continue; - } - - /* - * Do nothing else to cursors held over from a previous - * transaction. - */ - if (portal->createSubid == InvalidSubTransactionId) - continue; - + /* Is it a holdable portal created in the current xact? */ if ((portal->cursorOptions & CURSOR_OPT_HOLD) && + portal->createSubid != InvalidSubTransactionId && portal->status == PORTAL_READY) { /* @@ -484,12 +462,60 @@ AtCommit_Portals(void) * as not belonging to this transaction. */ portal->createSubid = InvalidSubTransactionId; + + result = true; } - else + } + + return result; +} + +/* + * Pre-commit processing for portals. + * + * Remove all non-holdable portals created in this transaction. + * Portals remaining from prior transactions should be left untouched. + * + * XXX This assumes that portals can be deleted in a random order, ie, + * no portal has a reference to any other (at least not one that will be + * exercised during deletion). I think this is okay at the moment, but + * we've had bugs of that ilk in the past. Keep a close eye on cursor + * references... + */ +void +AtCommit_Portals(void) +{ + HASH_SEQ_STATUS status; + PortalHashEnt *hentry; + + hash_seq_init(&status, PortalHashTable); + + while ((hentry = (PortalHashEnt *) hash_seq_search(&status)) != NULL) + { + Portal portal = hentry->portal; + + /* + * Do not touch active portals --- this can only happen in the + * case of a multi-transaction utility command, such as VACUUM. + * + * Note however that any resource owner attached to such a portal is + * still going to go away, so don't leave a dangling pointer. + */ + if (portal->status == PORTAL_ACTIVE) { - /* Zap all non-holdable portals */ - PortalDrop(portal, true); + portal->resowner = NULL; + continue; } + + /* + * Do nothing to cursors held over from a previous transaction + * (including holdable ones just frozen by CommitHoldablePortals). + */ + if (portal->createSubid == InvalidSubTransactionId) + continue; + + /* Zap all non-holdable portals */ + PortalDrop(portal, true); } } diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index 1aacccc811..81a665e247 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -6,7 +6,7 @@ * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/commands/trigger.h,v 1.52 2005/03/25 21:57:59 tgl Exp $ + * $PostgreSQL: pgsql/src/include/commands/trigger.h,v 1.53 2005/04/11 19:51:15 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -157,8 +157,8 @@ extern void ExecARUpdateTriggers(EState *estate, extern void AfterTriggerBeginXact(void); extern void AfterTriggerBeginQuery(void); extern void AfterTriggerEndQuery(EState *estate); -extern void AfterTriggerEndXact(void); -extern void AfterTriggerAbortXact(void); +extern void AfterTriggerFireDeferred(void); +extern void AfterTriggerEndXact(bool isCommit); extern void AfterTriggerBeginSubXact(void); extern void AfterTriggerEndSubXact(bool isCommit); diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h index 75e606c180..b8bcc33f58 100644 --- a/src/include/utils/portal.h +++ b/src/include/utils/portal.h @@ -39,7 +39,7 @@ * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/utils/portal.h,v 1.54 2004/12/31 22:03:46 pgsql Exp $ + * $PostgreSQL: pgsql/src/include/utils/portal.h,v 1.55 2005/04/11 19:51:16 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -182,6 +182,7 @@ typedef struct PortalData /* Prototypes for functions in utils/mmgr/portalmem.c */ extern void EnablePortalManager(void); +extern bool CommitHoldablePortals(void); extern void AtCommit_Portals(void); extern void AtAbort_Portals(void); extern void AtCleanup_Portals(void); diff --git a/src/test/regress/expected/portals.out b/src/test/regress/expected/portals.out index a46a14ce80..3f0e0cdd26 100644 --- a/src/test/regress/expected/portals.out +++ b/src/test/regress/expected/portals.out @@ -773,3 +773,38 @@ FETCH ALL FROM c; (15 rows) ROLLBACK; +-- +-- Test behavior of both volatile and stable functions inside a cursor; +-- in particular we want to see what happens during commit of a holdable +-- cursor +-- +create temp table tt1(f1 int); +create function count_tt1_v() returns int8 as +'select count(*) from tt1' language sql volatile; +create function count_tt1_s() returns int8 as +'select count(*) from tt1' language sql stable; +begin; +insert into tt1 values(1); +declare c1 cursor for select count_tt1_v(), count_tt1_s(); +insert into tt1 values(2); +fetch all from c1; + count_tt1_v | count_tt1_s +-------------+------------- + 2 | 1 +(1 row) + +rollback; +begin; +insert into tt1 values(1); +declare c2 cursor with hold for select count_tt1_v(), count_tt1_s(); +insert into tt1 values(2); +commit; +delete from tt1; +fetch all from c2; + count_tt1_v | count_tt1_s +-------------+------------- + 2 | 1 +(1 row) + +drop function count_tt1_v(); +drop function count_tt1_s(); diff --git a/src/test/regress/sql/portals.sql b/src/test/regress/sql/portals.sql index c7e29c3786..da4e3b0e3a 100644 --- a/src/test/regress/sql/portals.sql +++ b/src/test/regress/sql/portals.sql @@ -235,3 +235,46 @@ SELECT declares_cursor('AB%'); FETCH ALL FROM c; ROLLBACK; + +-- +-- Test behavior of both volatile and stable functions inside a cursor; +-- in particular we want to see what happens during commit of a holdable +-- cursor +-- + +create temp table tt1(f1 int); + +create function count_tt1_v() returns int8 as +'select count(*) from tt1' language sql volatile; + +create function count_tt1_s() returns int8 as +'select count(*) from tt1' language sql stable; + +begin; + +insert into tt1 values(1); + +declare c1 cursor for select count_tt1_v(), count_tt1_s(); + +insert into tt1 values(2); + +fetch all from c1; + +rollback; + +begin; + +insert into tt1 values(1); + +declare c2 cursor with hold for select count_tt1_v(), count_tt1_s(); + +insert into tt1 values(2); + +commit; + +delete from tt1; + +fetch all from c2; + +drop function count_tt1_v(); +drop function count_tt1_s(); -- 2.11.0