OSDN Git Service

Fix interaction between materializing holdable cursors and firing
authorTom Lane <tgl@sss.pgh.pa.us>
Mon, 11 Apr 2005 19:51:16 +0000 (19:51 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Mon, 11 Apr 2005 19:51:16 +0000 (19:51 +0000)
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
src/backend/commands/trigger.c
src/backend/utils/mmgr/portalmem.c
src/include/commands/trigger.h
src/include/utils/portal.h
src/test/regress/expected/portals.out
src/test/regress/sql/portals.sql

index b5bb3d5..4d896ef 100644 (file)
@@ -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();
index ef00943..8c8ed9e 100644 (file)
@@ -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
index 95e1972..fd8a737 100644 (file)
@@ -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);
        }
 }
 
index 1aacccc..81a665e 100644 (file)
@@ -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);
 
index 75e606c..b8bcc33 100644 (file)
@@ -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);
index a46a14c..3f0e0cd 100644 (file)
@@ -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();
index c7e29c3..da4e3b0 100644 (file)
@@ -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();