Ensure we have a snapshot when updating various system catalogs.

A few places that access system catalogs don't set up an active
snapshot before potentially accessing their TOAST tables.  To fix,
push an active snapshot just before each section of code that might
require accessing one of these TOAST tables, and pop it shortly
afterwards.  While at it, this commit adds some rather strict
assertions in an attempt to prevent such issues in the future.

Commit 16bf24e0e4 recently removed pg_replication_origin's TOAST
table in order to fix the same problem for that catalog.  On the
back-branches, those bugs are left in place.  We cannot easily
remove a catalog's TOAST table on released major versions, and only
replication origins with extremely long names are affected.  Given
the low severity of the issue, fixing older versions doesn't seem
worth the trouble of significantly modifying the patch.

Also, on v13 and v14, the aforementioned strict assertions have
been omitted because commit 2776922201, which added
HaveRegisteredOrActiveSnapshot(), was not back-patched.  While we
could probably back-patch it now, I've opted against it because it
seems unlikely that new TOAST snapshot issues will be introduced in
the oldest supported versions.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/18127-fe54b6a667f29658%40postgresql.org
Discussion: https://postgr.es/m/18309-c0bf914950c46692%40postgresql.org
Discussion: https://postgr.es/m/ZvMSUPOqUU-VNADN%40nathan
Backpatch-through: 13

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 8aaddcbb30be9105a02a033abd4062119d253db4..ba737c5b2ee6c354c21ee2d9e4e7d343f3ea187b 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -54,6 +54,7 @@
 #include "catalog/catalog.h"
 #include "catalog/pg_database.h"
 #include "catalog/pg_database_d.h"
+#include "catalog/pg_replication_origin.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "port/atomics.h"
@@ -227,6 +228,38 @@ static const int MultiXactStatusLock[MaxMultiXactStatus + 1] =
 #define TUPLOCK_from_mxstatus(status) \
                        (MultiXactStatusLock[(status)])
 
+/*
+ * Check that we have a valid snapshot if we might need TOAST access.
+ */
+static inline void
+AssertHasSnapshotForToast(Relation rel)
+{
+#ifdef USE_ASSERT_CHECKING
+
+       /* bootstrap mode in particular breaks this rule */
+       if (!IsNormalProcessingMode())
+               return;
+
+       /* if the relation doesn't have a TOAST table, we are good */
+       if (!OidIsValid(rel->rd_rel->reltoastrelid))
+               return;
+
+       /*
+        * Commit 16bf24e fixed accesses to pg_replication_origin without a
+        * an active snapshot by removing its TOAST table.  On older branches,
+        * these bugs are left in place.  Its only varlena column is roname (the
+        * replication origin name), so this is only a problem if the name
+        * requires out-of-line storage, which seems unlikely.  In any case,
+        * fixing it doesn't seem worth extra code churn on the back-branches.
+        */
+       if (RelationGetRelid(rel) == ReplicationOriginRelationId)
+               return;
+
+       Assert(HaveRegisteredOrActiveSnapshot());
+
+#endif                                                 /* USE_ASSERT_CHECKING */
+}
+
 /* ----------------------------------------------------------------
  *                                              heap support routines
  * ----------------------------------------------------------------
@@ -2047,6 +2080,8 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
        Assert(HeapTupleHeaderGetNatts(tup->t_data) <=
                   RelationGetNumberOfAttributes(relation));
 
+       AssertHasSnapshotForToast(relation);
+
        /*
         * Fill in tuple header fields and toast the tuple if necessary.
         *
@@ -2294,6 +2329,8 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
        /* currently not needed (thus unsupported) for heap_multi_insert() */
        AssertArg(!(options & HEAP_INSERT_NO_LOGICAL));
 
+       AssertHasSnapshotForToast(relation);
+
        needwal = RelationNeedsWAL(relation);
        saveFreeSpace = RelationGetTargetPageFreeSpace(relation,
                                                                                                   HEAP_DEFAULT_FILLFACTOR);
@@ -2697,6 +2734,8 @@ heap_delete(Relation relation, ItemPointer tid,
 
        Assert(ItemPointerIsValid(tid));
 
+       AssertHasSnapshotForToast(relation);
+
        /*
         * Forbid this during a parallel operation, lest it allocate a combo CID.
         * Other workers might need that combo CID for visibility checks, and we
@@ -3188,6 +3227,8 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
        Assert(HeapTupleHeaderGetNatts(newtup->t_data) <=
                   RelationGetNumberOfAttributes(relation));
 
+       AssertHasSnapshotForToast(relation);
+
        /*
         * Forbid this during a parallel operation, lest it allocate a combo CID.
         * Other workers might need that combo CID for visibility checks, and we

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 77aa99496faaad565351101a9aa5230ec5ac363b..1e3af0093b469e2b366d69f9e96f071f1b3e8ef2 100644 (file)
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3993,12 +3993,20 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
                                                                         get_rel_namespace(oldidx->tableId),
                                                                         false);
 
+               /*
+                * Swapping the indexes might involve TOAST table access, so ensure we
+                * have a valid snapshot.
+                */
+               PushActiveSnapshot(GetTransactionSnapshot());
+
                /*
                 * Swap old index with the new one.  This also marks the new one as
                 * valid and the old one as not valid.
                 */
                index_concurrently_swap(newidx->indexId, oldidx->indexId, oldName);
 
+               PopActiveSnapshot();
+
                /*
                 * Invalidate the relcache for the table, so that after this commit
                 * all sessions will refresh any cached plans that might reference the

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ab3ab7940838fb2c450fe6c594cef9024c52f02d..4f7be0247c2fa9b438c1b9b8eb02b06de0627dd1 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -18810,9 +18810,17 @@ ATExecDetachPartition(List **wqueue, AlteredTableInfo *tab, Relation rel,
                tab->rel = rel;
        }
 
+       /*
+        * Detaching the partition might involve TOAST table access, so ensure we
+        * have a valid snapshot.
+        */
+       PushActiveSnapshot(GetTransactionSnapshot());
+
        /* Do the final part of detaching */
        DetachPartitionFinalize(rel, partRel, concurrent, defaultPartOid);
 
+       PopActiveSnapshot();
+
        ObjectAddressSet(address, RelationRelationId, RelationGetRelid(partRel));
 
        /* keep our lock until commit */

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index c511a1322636c2f0d4efaeedcb4ac6c32f15301d..94c93c6e4b5864e38ffa045bacd8b1dae7355768 100644 (file)
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2315,6 +2315,12 @@ do_autovacuum(void)
                                                get_namespace_name(classForm->relnamespace),
                                                NameStr(classForm->relname))));
 
+               /*
+                * Deletion might involve TOAST table access, so ensure we have a
+                * valid snapshot.
+                */
+               PushActiveSnapshot(GetTransactionSnapshot());
+
                object.classId = RelationRelationId;
                object.objectId = relid;
                object.objectSubId = 0;
@@ -2327,6 +2333,7 @@ do_autovacuum(void)
                 * To commit the deletion, end current transaction and start a new
                 * one.  Note this also releases the locks we took.
                 */
+               PopActiveSnapshot();
                CommitTransactionCommand();
                StartTransactionCommand();
 

diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index dcc3fdf6c7685b0559440a4947e07051d0e80d22..155bf7c44ee7a9ea21c853c549b80c5127cdf6b8 100644 (file)
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -3794,8 +3794,16 @@ ApplyWorkerMain(Datum main_arg)
                        walrcv_startstreaming(LogRepWorkerWalRcvConn, &options);
 
                        StartTransactionCommand();
+
+                       /*
+                        * Updating pg_subscription might involve TOAST table access, so
+                        * ensure we have a valid snapshot.
+                        */
+                       PushActiveSnapshot(GetTransactionSnapshot());
+
                        UpdateTwoPhaseState(MySubscription->oid, LOGICALREP_TWOPHASE_STATE_ENABLED);
                        MySubscription->twophasestate = LOGICALREP_TWOPHASE_STATE_ENABLED;
+                       PopActiveSnapshot();
                        CommitTransactionCommand();
                }
                else
@@ -3848,7 +3856,15 @@ DisableSubscriptionAndExit(void)
 
        /* Disable the subscription */
        StartTransactionCommand();
+
+       /*
+        * Updating pg_subscription might involve TOAST table access, so ensure we
+        * have a valid snapshot.
+        */
+       PushActiveSnapshot(GetTransactionSnapshot());
+
        DisableSubscription(MySubscription->oid);
+       PopActiveSnapshot();
        CommitTransactionCommand();
 
        /* Notify the subscription has been disabled and exit */
@@ -3939,6 +3955,12 @@ clear_subscription_skip_lsn(XLogRecPtr finish_lsn)
                started_tx = true;
        }
 
+       /*
+        * Updating pg_subscription might involve TOAST table access, so ensure we
+        * have a valid snapshot.
+        */
+       PushActiveSnapshot(GetTransactionSnapshot());
+
        /*
         * Protect subskiplsn of pg_subscription from being concurrently updated
         * while clearing it.
@@ -3997,6 +4019,8 @@ clear_subscription_skip_lsn(XLogRecPtr finish_lsn)
        heap_freetuple(tup);
        table_close(rel, NoLock);
 
+       PopActiveSnapshot();
+
        if (started_tx)
                CommitTransactionCommand();
 }