Fix a couple of snapshot management bugs in the new ResourceOwner world:

non-writable large objects need to have their snapshots registered on the
transaction resowner, not the current portal's, because it must persist until
the large object is closed (which the portal does not).  Also, ensure that the
serializable snapshot is recorded by the transaction resource owner too, even
when a subtransaction has changed the current resource owner before
serializable is taken.

Per bug reports from Pavan Deolasee.

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index f32df1a88888253b022a6a2d68c39d572a0c1635..5187b9ffd81b229d932a7d184693bed42a86c070 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1667,6 +1667,9 @@ CommitTransaction(void)
        /* Clean up the relation cache */
        AtEOXact_RelationCache(true);
 
+       /* Clean up the snapshot manager */
+       AtEarlyCommit_Snapshot();
+
        /*
         * Make catalog changes visible to all backends.  This has to happen after
         * relcache references are dropped (see comments for
@@ -1906,6 +1909,9 @@ PrepareTransaction(void)
        /* Clean up the relation cache */
        AtEOXact_RelationCache(true);
 
+       /* Clean up the snapshot manager */
+       AtEarlyCommit_Snapshot();
+
        /* notify and flatfiles don't need a postprepare call */
 
        PostPrepare_PgStat();

diff --git a/src/backend/storage/large_object/inv_api.c b/src/backend/storage/large_object/inv_api.c
index 494d3870a535b68616a4eb9768cb5a85be14ea75..7ee080f3a0e0e4b28a86a48b4ec3f4c90f97ac66 100644 (file)
--- a/src/backend/storage/large_object/inv_api.c
+++ b/src/backend/storage/large_object/inv_api.c
@@ -247,7 +247,13 @@ inv_open(Oid lobjId, int flags, MemoryContext mcxt)
        }
        else if (flags & INV_READ)
        {
-               retval->snapshot = RegisterSnapshot(GetActiveSnapshot());
+               /*
+                * We must register the snapshot in TopTransaction's resowner,
+                * because it must stay alive until the LO is closed rather than until
+                * the current portal shuts down.
+                */
+               retval->snapshot = RegisterSnapshotOnOwner(GetActiveSnapshot(),
+                                                                                                  TopTransactionResourceOwner);
                retval->flags = IFS_RDLOCK;
        }
        else
@@ -270,8 +276,11 @@ void
 inv_close(LargeObjectDesc *obj_desc)
 {
        Assert(PointerIsValid(obj_desc));
+
        if (obj_desc->snapshot != SnapshotNow)
-               UnregisterSnapshot(obj_desc->snapshot);
+               UnregisterSnapshotFromOwner(obj_desc->snapshot,
+                                                                       TopTransactionResourceOwner);
+
        pfree(obj_desc);
 }
 

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 36d571ebb54753ccaab47ac0a806ab7bcd8ed1f3..398916a1ef267b0830d57dbd262566d3674b6375 100644 (file)
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -136,7 +136,8 @@ GetTransactionSnapshot(void)
                 */
                if (IsXactIsoLevelSerializable)
                {
-                       CurrentSnapshot = RegisterSnapshot(CurrentSnapshot);
+                       CurrentSnapshot = RegisterSnapshotOnOwner(CurrentSnapshot,
+                                                                                                         TopTransactionResourceOwner);
                        registered_serializable = true;
                }
 
@@ -345,14 +346,27 @@ ActiveSnapshotSet(void)
 
 /*
  * RegisterSnapshot
- *             Register a snapshot as being in use
+ *             Register a snapshot as being in use by the current resource owner
  *
  * If InvalidSnapshot is passed, it is not registered.
  */
 Snapshot
 RegisterSnapshot(Snapshot snapshot)
 {
-       Snapshot        snap;
+       if (snapshot == InvalidSnapshot)
+               return InvalidSnapshot;
+
+       return RegisterSnapshotOnOwner(snapshot, CurrentResourceOwner);
+}
+
+/*
+ * RegisterSnapshotOnOwner
+ *             As above, but use the specified resource owner
+ */
+Snapshot
+RegisterSnapshotOnOwner(Snapshot snapshot, ResourceOwner owner)
+{
+       Snapshot                snap;
 
        if (snapshot == InvalidSnapshot)
                return InvalidSnapshot;
@@ -361,9 +375,9 @@ RegisterSnapshot(Snapshot snapshot)
        snap = snapshot->copied ? snapshot : CopySnapshot(snapshot);
 
        /* and tell resowner.c about it */
-       ResourceOwnerEnlargeSnapshots(CurrentResourceOwner);
+       ResourceOwnerEnlargeSnapshots(owner);
        snap->regd_count++;
-       ResourceOwnerRememberSnapshot(CurrentResourceOwner, snap);
+       ResourceOwnerRememberSnapshot(owner, snap);
 
        RegisteredSnapshots++;
 
@@ -379,6 +393,19 @@ RegisterSnapshot(Snapshot snapshot)
  */
 void
 UnregisterSnapshot(Snapshot snapshot)
+{
+       if (snapshot == NULL)
+               return;
+
+       UnregisterSnapshotFromOwner(snapshot, CurrentResourceOwner);
+}
+
+/*
+ * UnregisterSnapshotFromOwner
+ *             As above, but use the specified resource owner
+ */
+void
+UnregisterSnapshotFromOwner(Snapshot snapshot, ResourceOwner owner)
 {
        if (snapshot == NULL)
                return;
@@ -386,7 +413,7 @@ UnregisterSnapshot(Snapshot snapshot)
        Assert(snapshot->regd_count > 0);
        Assert(RegisteredSnapshots > 0);
 
-       ResourceOwnerForgetSnapshot(CurrentResourceOwner, snapshot);
+       ResourceOwnerForgetSnapshot(owner, snapshot);
        RegisteredSnapshots--;
        if (--snapshot->regd_count == 0 && snapshot->active_count == 0)
        {
@@ -463,6 +490,26 @@ AtSubAbort_Snapshot(int level)
        SnapshotResetXmin();
 }
 
+/*
+ * AtEarlyCommit_Snapshot
+ *
+ * Snapshot manager's cleanup function, to be called on commit, before
+ * doing resowner.c resource release.
+ */
+void
+AtEarlyCommit_Snapshot(void)
+{
+       /*
+        * On a serializable transaction we must unregister our private refcount to
+        * the serializable snapshot.
+        */
+       if (registered_serializable)
+               UnregisterSnapshotFromOwner(CurrentSnapshot,
+                                                                       TopTransactionResourceOwner);
+       registered_serializable = false;
+
+}
+
 /*
  * AtEOXact_Snapshot
  *             Snapshot manager's cleanup function for end of transaction
@@ -475,13 +522,6 @@ AtEOXact_Snapshot(bool isCommit)
        {
                ActiveSnapshotElt       *active;
 
-               /*
-                * On a serializable snapshot we must first unregister our private
-                * refcount to the serializable snapshot.
-                */
-               if (registered_serializable)
-                       UnregisterSnapshot(CurrentSnapshot);
-
                if (RegisteredSnapshots != 0)
                        elog(WARNING, "%d registered snapshots seem to remain after cleanup",
                                 RegisteredSnapshots);

diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index 392952da430d36234b0b16c1eae35d1144e6cf33..4b4e915c0f903ca1c593e398d242c451a0918d84 100644 (file)
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -13,6 +13,7 @@
 #ifndef SNAPMGR_H
 #define SNAPMGR_H
 
+#include "utils/resowner.h"
 #include "utils/snapshot.h"
 
 
@@ -34,9 +35,12 @@ extern bool ActiveSnapshotSet(void);
 
 extern Snapshot RegisterSnapshot(Snapshot snapshot);
 extern void UnregisterSnapshot(Snapshot snapshot);
+extern Snapshot RegisterSnapshotOnOwner(Snapshot snapshot, ResourceOwner owner);
+extern void UnregisterSnapshotFromOwner(Snapshot snapshot, ResourceOwner owner);
 
 extern void AtSubCommit_Snapshot(int level);
 extern void AtSubAbort_Snapshot(int level);
+extern void AtEarlyCommit_Snapshot(void);
 extern void AtEOXact_Snapshot(bool isCommit);
 
 #endif /* SNAPMGR_H */

diff --git a/src/test/regress/input/largeobject.source b/src/test/regress/input/largeobject.source
index 1d62caa3eaf0488028021a3cf5715ffa6f126d10..46ba9261ac5fe9426217da76d2b769103976e23b 100644 (file)
--- a/src/test/regress/input/largeobject.source
+++ b/src/test/regress/input/largeobject.source
@@ -83,6 +83,11 @@ SELECT lo_close(fd) FROM lotest_stash_values;
 
 END;
 
+-- Test resource management
+BEGIN;
+SELECT lo_open(loid, x'40000'::int) from lotest_stash_values;
+ABORT;
+
 -- Test truncation.
 BEGIN;
 UPDATE lotest_stash_values SET fd=lo_open(loid, CAST(x'20000' | x'40000' AS integer));

diff --git a/src/test/regress/output/largeobject.source b/src/test/regress/output/largeobject.source
index 36b51fdccddf548437a580591995d5a193236329..9d69f6c913e2ebd7bfade32b9546450280594eae 100644 (file)
--- a/src/test/regress/output/largeobject.source
+++ b/src/test/regress/output/largeobject.source
@@ -116,6 +116,15 @@ SELECT lo_close(fd) FROM lotest_stash_values;
 (1 row)
 
 END;
+-- Test resource management
+BEGIN;
+SELECT lo_open(loid, x'40000'::int) from lotest_stash_values;
+ lo_open 
+---------
+       0
+(1 row)
+
+ABORT;
 -- Test truncation.
 BEGIN;
 UPDATE lotest_stash_values SET fd=lo_open(loid, CAST(x'20000' | x'40000' AS integer));

diff --git a/src/test/regress/output/largeobject_1.source b/src/test/regress/output/largeobject_1.source
index f1157e45718890582013a5da5e44e52cfc00a70c..1fbc29c25171d6e2bc02b4d49012e990f1190a09 100644 (file)
--- a/src/test/regress/output/largeobject_1.source
+++ b/src/test/regress/output/largeobject_1.source
@@ -116,6 +116,15 @@ SELECT lo_close(fd) FROM lotest_stash_values;
 (1 row)
 
 END;
+-- Test resource management
+BEGIN;
+SELECT lo_open(loid, x'40000'::int) from lotest_stash_values;
+ lo_open 
+---------
+       0
+(1 row)
+
+ABORT;
 -- Test truncation.
 BEGIN;
 UPDATE lotest_stash_values SET fd=lo_open(loid, CAST(x'20000' | x'40000' AS integer));