Fix snapshot management, take two.

Partially revert the previous patch I installed and replace it with a more
general fix: any time a snapshot is pushed as Active, we need to ensure that it
will not be modified in the future.  This means that if the same snapshot is
used as CurrentSnapshot, it needs to be copied separately.  This affects
serializable transactions only, because CurrentSnapshot has already been copied
by RegisterSnapshot and so PushActiveSnapshot does not think it needs another
copy.  However, CommandCounterIncrement would modify CurrentSnapshot, whereas
ActiveSnapshots must not have their command counters incremented.

I say "partially" because the regression test I added for the previous bug
has been kept.

(This restores 8.3 behavior, because before snapmgr.c existed, any snapshot set
as Active was copied.)

Per bug report from Stuart Bishop in
6bc73d4c0910042358k3d1adff3qa36f8df75198ecea@mail.gmail.com

diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
index d31fde65b6b3b50ffd349ff7e4df080bddd15398..f1ae01908a944b0e15ee7d276e1186520de8e3b3 100644 (file)
--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -14,7 +14,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.79.2.1 2009/10/02 17:58:21 alvherre Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.79.2.2 2009/10/07 16:27:28 alvherre Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -47,7 +47,6 @@ PerformCursorOpen(PlannedStmt *stmt, ParamListInfo params,
        DeclareCursorStmt *cstmt = (DeclareCursorStmt *) stmt->utilityStmt;
        Portal          portal;
        MemoryContext oldContext;
-       Snapshot        snapshot;
 
        if (cstmt == NULL || !IsA(cstmt, DeclareCursorStmt))
                elog(ERROR, "PerformCursorOpen called for non-cursor query");
@@ -119,18 +118,10 @@ PerformCursorOpen(PlannedStmt *stmt, ParamListInfo params,
                        portal->cursorOptions |= CURSOR_OPT_NO_SCROLL;
        }
 
-       /*
-        * Set up snapshot for portal.  Note that we need a fresh, independent copy
-        * of the snapshot because we don't want it to be modified by future
-        * CommandCounterIncrement calls.  We do not register it, because
-        * portalmem.c will take care of that internally.
-        */
-       snapshot = CopySnapshot(GetActiveSnapshot());
-
        /*
         * Start execution, inserting parameters if any.
         */
-       PortalStart(portal, params, snapshot);
+       PortalStart(portal, params, GetActiveSnapshot());
 
        Assert(portal->strategy == PORTAL_ONE_SELECT);
 

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 12ca81616d17dc6102e1893abd4c3990e5eaa249..9c06124ba952495b00a76c0310aa9a98c4c74e84 100644 (file)
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -19,7 +19,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/time/snapmgr.c,v 1.10.2.1 2009/10/02 17:58:21 alvherre Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/time/snapmgr.c,v 1.10.2.2 2009/10/07 16:27:28 alvherre Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -104,6 +104,7 @@ bool                FirstSnapshotSet = false;
 static bool registered_serializable = false;
 
 
+static Snapshot CopySnapshot(Snapshot snapshot);
 static void FreeSnapshot(Snapshot snapshot);
 static void SnapshotResetXmin(void);
 
@@ -191,7 +192,7 @@ SnapshotSetCommandId(CommandId curcid)
  * The copy is palloc'd in TopTransactionContext and has initial refcounts set
  * to 0.  The returned snapshot has the copied flag set.
  */
-Snapshot
+static Snapshot
 CopySnapshot(Snapshot snapshot)
 {
        Snapshot        newsnap;
@@ -254,8 +255,9 @@ FreeSnapshot(Snapshot snapshot)
  * PushActiveSnapshot
  *             Set the given snapshot as the current active snapshot
  *
- * If this is the first use of this snapshot, create a new long-lived copy with
- * active refcount=1.  Otherwise, only increment the refcount.
+ * If the passed snapshot is a statically-allocated one, or it is possibly
+ * subject to a future command counter update, create a new long-lived copy
+ * with active refcount=1.  Otherwise, only increment the refcount.
  */
 void
 PushActiveSnapshot(Snapshot snap)
@@ -265,8 +267,16 @@ PushActiveSnapshot(Snapshot snap)
        Assert(snap != InvalidSnapshot);
 
        newactive = MemoryContextAlloc(TopTransactionContext, sizeof(ActiveSnapshotElt));
-       /* Static snapshot?  Create a persistent copy */
-       newactive->as_snap = snap->copied ? snap : CopySnapshot(snap);
+
+       /*
+        * Checking SecondarySnapshot is probably useless here, but it seems better
+        * to be sure.
+        */
+       if (snap == CurrentSnapshot || snap == SecondarySnapshot || !snap->copied)
+               newactive->as_snap = CopySnapshot(snap);
+       else
+               newactive->as_snap = snap;
+
        newactive->as_next = ActiveSnapshot;
        newactive->as_level = GetCurrentTransactionNestLevel();
 

diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index d2186116023e6f0f216c5b4f4e693e0bef7546e3..b0d0f5149d8a75356db27740d711581be00895f6 100644 (file)
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/utils/snapmgr.h,v 1.5.2.1 2009/10/02 17:58:21 alvherre Exp $
+ * $PostgreSQL: pgsql/src/include/utils/snapmgr.h,v 1.5.2.2 2009/10/07 16:27:29 alvherre Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -26,7 +26,6 @@ extern TransactionId RecentGlobalXmin;
 extern Snapshot GetTransactionSnapshot(void);
 extern Snapshot GetLatestSnapshot(void);
 extern void SnapshotSetCommandId(CommandId curcid);
-extern Snapshot CopySnapshot(Snapshot snapshot);
 
 extern void PushActiveSnapshot(Snapshot snapshot);
 extern void PushUpdatedSnapshot(Snapshot snapshot);

diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 2a5cb909d7027ec9ee0b6d9e940354c0e107378a..c0b7645600dcc5fbf257a6dfaa47b036ad1f77b5 100644 (file)
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -537,6 +537,37 @@ NOTICE:  row 1 not changed
 NOTICE:  row 2 not changed
 DROP TABLE trigger_test;
 DROP FUNCTION mytrigger();
+-- Test snapshot management in serializable transactions involving triggers
+-- per bug report in 6bc73d4c0910042358k3d1adff3qa36f8df75198ecea@mail.gmail.com
+CREATE FUNCTION serializable_update_trig() RETURNS trigger LANGUAGE plpgsql AS
+$$
+declare
+       rec record;
+begin
+       new.description = 'updated in trigger';
+       return new;
+end;
+$$;
+CREATE TABLE serializable_update_tab (
+       id int,
+       filler  text,
+       description text
+);
+CREATE TRIGGER serializable_update_trig BEFORE UPDATE ON serializable_update_tab
+       FOR EACH ROW EXECUTE PROCEDURE serializable_update_trig();
+INSERT INTO serializable_update_tab SELECT a, repeat('xyzxz', 100), 'new'
+       FROM generate_series(1, 50) a;
+BEGIN;
+SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+UPDATE serializable_update_tab SET description = 'no no', id = 1 WHERE id = 1;
+COMMIT;
+SELECT description FROM serializable_update_tab WHERE id = 1;
+    description     
+--------------------
+ updated in trigger
+(1 row)
+
+DROP TABLE serializable_update_tab;
 -- minimal update trigger
 CREATE TABLE min_updates_test (
        f1      text,

diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 8530030ef8b8083ceb07e71919881b654775dbb3..878adbb0d4b5c44b9c296103b7a67f1751e4dd81 100644 (file)
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -416,6 +416,36 @@ DROP TABLE trigger_test;
 
 DROP FUNCTION mytrigger();
 
+-- Test snapshot management in serializable transactions involving triggers
+-- per bug report in 6bc73d4c0910042358k3d1adff3qa36f8df75198ecea@mail.gmail.com
+CREATE FUNCTION serializable_update_trig() RETURNS trigger LANGUAGE plpgsql AS
+$$
+declare
+       rec record;
+begin
+       new.description = 'updated in trigger';
+       return new;
+end;
+$$;
+
+CREATE TABLE serializable_update_tab (
+       id int,
+       filler  text,
+       description text
+);
+
+CREATE TRIGGER serializable_update_trig BEFORE UPDATE ON serializable_update_tab
+       FOR EACH ROW EXECUTE PROCEDURE serializable_update_trig();
+
+INSERT INTO serializable_update_tab SELECT a, repeat('xyzxz', 100), 'new'
+       FROM generate_series(1, 50) a;
+
+BEGIN;
+SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+UPDATE serializable_update_tab SET description = 'no no', id = 1 WHERE id = 1;
+COMMIT;
+SELECT description FROM serializable_update_tab WHERE id = 1;
+DROP TABLE serializable_update_tab;
 
 -- minimal update trigger