On Tue, Oct 08, 2024 at 01:50:52PM -0500, Nathan Bossart wrote: > 0002 could probably use some more commentary, but otherwise I think it is > in decent shape. You (Michael) seem to be implying that I should > back-patch the actual fixes and only apply the new assertions to v18. Am I > understanding you correctly?
Here is a reorganized patch set. 0001 would be back-patched, but the others would only be applied to v18. -- nathan
>From 6792eeb656e8fc707d85dc07595e2183853d2ce5 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Mon, 14 Oct 2024 14:43:26 -0500 Subject: [PATCH v3 1/3] Ensure we have a snapshot when updating various system catalogs. Reported-by: Alexander Lakhin Reviewed-by: Michael Paquier 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: FIXME --- src/backend/commands/tablecmds.c | 13 +++++++++++ src/backend/replication/logical/tablesync.c | 21 ++++++++++++++++++ src/backend/replication/logical/worker.c | 24 +++++++++++++++++++++ 3 files changed, 58 insertions(+) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 1ccc80087c..c6f82e93b9 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -19283,9 +19283,22 @@ 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. We only expect to get here without a snapshot + * in the concurrent path. + */ + if (concurrent) + PushActiveSnapshot(GetTransactionSnapshot()); + else + Assert(HaveRegisteredOrActiveSnapshot()); + /* Do the final part of detaching */ DetachPartitionFinalize(rel, partRel, concurrent, defaultPartOid); + if (concurrent) + PopActiveSnapshot(); + ObjectAddressSet(address, RelationRelationId, RelationGetRelid(partRel)); /* keep our lock until commit */ diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c index e03e761392..d3fbb18438 100644 --- a/src/backend/replication/logical/tablesync.c +++ b/src/backend/replication/logical/tablesync.c @@ -377,6 +377,12 @@ process_syncing_tables_for_sync(XLogRecPtr current_lsn) replorigin_session_origin_lsn = InvalidXLogRecPtr; replorigin_session_origin_timestamp = 0; + /* + * Updating pg_replication_origin might involve TOAST table access, so + * ensure we have a valid snapshot. + */ + PushActiveSnapshot(GetTransactionSnapshot()); + /* * Drop the tablesync's origin tracking if exists. * @@ -387,6 +393,8 @@ process_syncing_tables_for_sync(XLogRecPtr current_lsn) */ replorigin_drop_by_name(originname, true, false); + PopActiveSnapshot(); + finish_sync_worker(); } else @@ -483,6 +491,12 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn) started_tx = true; } + /* + * Updating pg_replication_origin might involve TOAST table + * access, so ensure we have a valid snapshot. + */ + PushActiveSnapshot(GetTransactionSnapshot()); + /* * Remove the tablesync origin tracking if exists. * @@ -500,6 +514,8 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn) sizeof(originname)); replorigin_drop_by_name(originname, true, false); + PopActiveSnapshot(); + /* * Update the state to READY only after the origin cleanup. */ @@ -1455,8 +1471,13 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos) * Then advance to the LSN got from walrcv_create_slot. This is WAL * logged for the purpose of recovery. Locks are to prevent the * replication origin from vanishing while advancing. + * + * Updating pg_replication_origin might involve TOAST table access, so + * ensure we have a valid snapshot. */ + PushActiveSnapshot(GetTransactionSnapshot()); originid = replorigin_create(originname); + PopActiveSnapshot(); LockRelationOid(ReplicationOriginRelationId, RowExclusiveLock); replorigin_advance(originid, *origin_startpos, InvalidXLogRecPtr, diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index 925dff9cc4..804e5dbb75 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -4604,8 +4604,16 @@ run_apply_worker() 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 @@ -4821,7 +4829,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(); /* Ensure we remove no-longer-useful entry for worker's start time */ @@ -4925,6 +4941,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. @@ -4983,6 +5005,8 @@ clear_subscription_skip_lsn(XLogRecPtr finish_lsn) heap_freetuple(tup); table_close(rel, NoLock); + PopActiveSnapshot(); + if (started_tx) CommitTransactionCommand(); } -- 2.39.5 (Apple Git-154)
>From 4294f5f3c81ec4365ec5bd8bfa80069496732dc5 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Mon, 14 Oct 2024 14:50:59 -0500 Subject: [PATCH v3 2/3] Introduce RelationGetToastRelid macro. Discussion: https://postgr.es/m/ZvMSUPOqUU-VNADN%40nathan --- contrib/amcheck/verify_heapam.c | 6 +++--- src/backend/access/common/toast_internals.c | 2 +- src/backend/access/heap/heaptoast.c | 6 +++--- src/backend/catalog/heap.c | 2 +- src/backend/catalog/index.c | 2 +- src/backend/catalog/toasting.c | 2 +- src/backend/commands/cluster.c | 19 ++++++++++--------- src/backend/commands/indexcmds.c | 4 ++-- src/backend/commands/tablecmds.c | 8 ++++---- src/backend/commands/vacuum.c | 2 +- .../replication/logical/reorderbuffer.c | 4 ++-- src/backend/utils/adt/dbsize.c | 4 ++-- src/include/utils/rel.h | 6 ++++++ 13 files changed, 37 insertions(+), 30 deletions(-) diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c index f2526ed63a..78316daacd 100644 --- a/contrib/amcheck/verify_heapam.c +++ b/contrib/amcheck/verify_heapam.c @@ -367,12 +367,12 @@ verify_heapam(PG_FUNCTION_ARGS) } /* Optionally open the toast relation, if any. */ - if (ctx.rel->rd_rel->reltoastrelid && check_toast) + if (RelationGetToastRelid(ctx.rel) && check_toast) { int offset; /* Main relation has associated toast relation */ - ctx.toast_rel = table_open(ctx.rel->rd_rel->reltoastrelid, + ctx.toast_rel = table_open(RelationGetToastRelid(ctx.rel), AccessShareLock); offset = toast_open_indexes(ctx.toast_rel, AccessShareLock, @@ -1721,7 +1721,7 @@ check_tuple_attribute(HeapCheckContext *ctx) } /* The relation better have a toast table */ - if (!ctx->rel->rd_rel->reltoastrelid) + if (!OidIsValid(RelationGetToastRelid(ctx->rel))) { report_corruption(ctx, psprintf("toast value %u is external but relation has no toast relation", diff --git a/src/backend/access/common/toast_internals.c b/src/backend/access/common/toast_internals.c index 90d0654e62..39d93d12f2 100644 --- a/src/backend/access/common/toast_internals.c +++ b/src/backend/access/common/toast_internals.c @@ -151,7 +151,7 @@ toast_save_datum(Relation rel, Datum value, * uniqueness of the OID we assign to the toasted item, even though it has * additional columns besides OID. */ - toastrel = table_open(rel->rd_rel->reltoastrelid, RowExclusiveLock); + toastrel = table_open(RelationGetToastRelid(rel), RowExclusiveLock); toasttupDesc = toastrel->rd_att; /* Open all the toast indexes and look for the valid one */ diff --git a/src/backend/access/heap/heaptoast.c b/src/backend/access/heap/heaptoast.c index a420e16530..cf6f3dff53 100644 --- a/src/backend/access/heap/heaptoast.c +++ b/src/backend/access/heap/heaptoast.c @@ -213,7 +213,7 @@ heap_toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup, * XXX maybe the threshold should be less than maxDataLen? */ if (toast_attr[biggest_attno].tai_size > maxDataLen && - rel->rd_rel->reltoastrelid != InvalidOid) + OidIsValid(RelationGetToastRelid(rel))) toast_tuple_externalize(&ttc, biggest_attno, options); } @@ -224,7 +224,7 @@ heap_toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup, */ while (heap_compute_data_size(tupleDesc, toast_values, toast_isnull) > maxDataLen && - rel->rd_rel->reltoastrelid != InvalidOid) + OidIsValid(RelationGetToastRelid(rel))) { int biggest_attno; @@ -259,7 +259,7 @@ heap_toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup, while (heap_compute_data_size(tupleDesc, toast_values, toast_isnull) > maxDataLen && - rel->rd_rel->reltoastrelid != InvalidOid) + OidIsValid(RelationGetToastRelid(rel))) { int biggest_attno; diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 0078a12f26..1b912488d6 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -3073,7 +3073,7 @@ heap_truncate_one_rel(Relation rel) RelationTruncateIndexes(rel); /* If there is a toast table, truncate that too */ - toastrelid = rel->rd_rel->reltoastrelid; + toastrelid = RelationGetToastRelid(rel); if (OidIsValid(toastrelid)) { Relation toastrel = table_open(toastrelid, AccessExclusiveLock); diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 12822d0b14..3eab421a16 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -3930,7 +3930,7 @@ reindex_relation(const ReindexStmt *stmt, Oid relid, int flags, get_namespace_name(RelationGetNamespace(rel)), RelationGetRelationName(rel)); - toast_relid = rel->rd_rel->reltoastrelid; + toast_relid = RelationGetToastRelid(rel); /* * Get the list of index OIDs for this relation. (We trust the relcache diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c index ad3082c62a..cf47e0960e 100644 --- a/src/backend/catalog/toasting.c +++ b/src/backend/catalog/toasting.c @@ -149,7 +149,7 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid, /* * Is it already toasted? */ - if (rel->rd_rel->reltoastrelid != InvalidOid) + if (OidIsValid(RelationGetToastRelid(rel))) return false; /* diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 78f96789b0..20da5201b6 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -780,7 +780,7 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, Oid NewAccessMethod, * Note that NewHeapCreateToastTable ends with CommandCounterIncrement, so * that the TOAST table will be visible for insertion. */ - toastid = OldHeap->rd_rel->reltoastrelid; + toastid = RelationGetToastRelid(OldHeap); if (OidIsValid(toastid)) { /* keep the existing toast table's reloptions, if any */ @@ -870,8 +870,8 @@ copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, bool verbose, * We don't need to open the toast relation here, just lock it. The lock * will be held till end of transaction. */ - if (OldHeap->rd_rel->reltoastrelid) - LockRelationOid(OldHeap->rd_rel->reltoastrelid, AccessExclusiveLock); + if (RelationGetToastRelid(OldHeap)) + LockRelationOid(RelationGetToastRelid(OldHeap), AccessExclusiveLock); /* * If both tables have TOAST tables, perform toast swap by content. It is @@ -880,7 +880,8 @@ copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, bool verbose, * swap by links. This is okay because swap by content is only essential * for system catalogs, and we don't support schema changes for them. */ - if (OldHeap->rd_rel->reltoastrelid && NewHeap->rd_rel->reltoastrelid) + if (OidIsValid(RelationGetToastRelid(OldHeap)) && + OidIsValid(RelationGetToastRelid(NewHeap))) { *pSwapToastByContent = true; @@ -902,7 +903,7 @@ copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, bool verbose, * work for values copied over from the old toast table, but not for * any values that we toast which were previously not toasted.) */ - NewHeap->rd_toastoid = OldHeap->rd_rel->reltoastrelid; + NewHeap->rd_toastoid = RelationGetToastRelid(OldHeap); } else *pSwapToastByContent = false; @@ -1581,19 +1582,19 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, Relation newrel; newrel = table_open(OIDOldHeap, NoLock); - if (OidIsValid(newrel->rd_rel->reltoastrelid)) + if (OidIsValid(RelationGetToastRelid(newrel))) { Oid toastidx; char NewToastName[NAMEDATALEN]; /* Get the associated valid index to be renamed */ - toastidx = toast_get_valid_index(newrel->rd_rel->reltoastrelid, + toastidx = toast_get_valid_index(RelationGetToastRelid(newrel), NoLock); /* rename the toast table ... */ snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u", OIDOldHeap); - RenameRelationInternal(newrel->rd_rel->reltoastrelid, + RenameRelationInternal(RelationGetToastRelid(newrel), NewToastName, true, false); /* ... and its valid index too. */ @@ -1609,7 +1610,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, * that is updated as part of RenameRelationInternal. */ CommandCounterIncrement(); - ResetRelRewrite(newrel->rd_rel->reltoastrelid); + ResetRelRewrite(RelationGetToastRelid(newrel)); } relation_close(newrel, NoLock); } diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index e33ad81529..b7ddced7f0 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -3680,9 +3680,9 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein } /* Also add the toast indexes */ - if (OidIsValid(heapRelation->rd_rel->reltoastrelid)) + if (OidIsValid(RelationGetToastRelid(heapRelation))) { - Oid toastOid = heapRelation->rd_rel->reltoastrelid; + Oid toastOid = RelationGetToastRelid(heapRelation); Relation toastRelation = table_open(toastOid, ShareUpdateExclusiveLock); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index c6f82e93b9..cf076dec5d 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -2140,7 +2140,7 @@ ExecuteTruncateGuts(List *explicit_rels, /* * The same for the toast table, if any. */ - toast_relid = rel->rd_rel->reltoastrelid; + toast_relid = RelationGetToastRelid(rel); if (OidIsValid(toast_relid)) { Relation toastrel = relation_open(toast_relid, @@ -15161,10 +15161,10 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation, ReleaseSysCache(tuple); /* repeat the whole exercise for the toast table, if there's one */ - if (OidIsValid(rel->rd_rel->reltoastrelid)) + if (OidIsValid(RelationGetToastRelid(rel))) { Relation toastrel; - Oid toastid = rel->rd_rel->reltoastrelid; + Oid toastid = RelationGetToastRelid(rel); toastrel = table_open(toastid, lockmode); @@ -15253,7 +15253,7 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode) return; } - reltoastrelid = rel->rd_rel->reltoastrelid; + reltoastrelid = RelationGetToastRelid(rel); /* Fetch the list of indexes on toast relation if necessary */ if (OidIsValid(reltoastrelid)) { diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index ac8f5d9c25..08bd489aed 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -2187,7 +2187,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, if ((params->options & VACOPT_PROCESS_TOAST) != 0 && ((params->options & VACOPT_FULL) == 0 || (params->options & VACOPT_PROCESS_MAIN) == 0)) - toast_relid = rel->rd_rel->reltoastrelid; + toast_relid = RelationGetToastRelid(rel); else toast_relid = InvalidOid; diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 22bcf171ff..c9d9590108 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -4822,10 +4822,10 @@ ReorderBufferToastReplace(ReorderBuffer *rb, ReorderBufferTXN *txn, desc = RelationGetDescr(relation); - toast_rel = RelationIdGetRelation(relation->rd_rel->reltoastrelid); + toast_rel = RelationIdGetRelation(RelationGetToastRelid(relation)); if (!RelationIsValid(toast_rel)) elog(ERROR, "could not open toast relation with OID %u (base relation \"%s\")", - relation->rd_rel->reltoastrelid, RelationGetRelationName(relation)); + RelationGetToastRelid(relation), RelationGetRelationName(relation)); toast_desc = RelationGetDescr(toast_rel); diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c index e63e99c141..a3597be9bc 100644 --- a/src/backend/utils/adt/dbsize.c +++ b/src/backend/utils/adt/dbsize.c @@ -436,8 +436,8 @@ calculate_table_size(Relation rel) /* * Size of toast relation */ - if (OidIsValid(rel->rd_rel->reltoastrelid)) - size += calculate_toast_table_size(rel->rd_rel->reltoastrelid); + if (OidIsValid(RelationGetToastRelid(rel))) + size += calculate_toast_table_size(RelationGetToastRelid(rel)); return size; } diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index 8700204953..6631b37d97 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -504,6 +504,12 @@ typedef struct ViewOptions */ #define RelationGetRelid(relation) ((relation)->rd_id) +/* + * RelationGetToastRelid + * Returns the OID of the relation's TOAST table (or InvalidOid if none) + */ +#define RelationGetToastRelid(relation) ((relation)->rd_rel->reltoastrelid) + /* * RelationGetNumberOfAttributes * Returns the total number of attributes in a relation. -- 2.39.5 (Apple Git-154)
>From 1484561a5a9cfe070293d3cbb8528bac1634f185 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Mon, 14 Oct 2024 14:53:36 -0500 Subject: [PATCH v3 3/3] Assert that we have a valid snapshot if we might need TOAST access. Suggested-by: Michael Paquier Reviewed-by: Michael Paquier Discussion: https://postgr.es/m/ZvMSUPOqUU-VNADN%40nathan --- src/backend/access/heap/heapam.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index da5e656a08..d51525a85c 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -210,6 +210,23 @@ 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) +{ + /* bootstrap mode in particular breaks this rule */ + if (!IsNormalProcessingMode()) + return; + + /* if the relation doesn't have a TOAST table, we are good */ + if (!OidIsValid(RelationGetToastRelid(rel))) + return; + + Assert(HaveRegisteredOrActiveSnapshot()); +} + /* ---------------------------------------------------------------- * heap support routines * ---------------------------------------------------------------- @@ -1995,6 +2012,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. * @@ -2272,6 +2291,8 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, /* currently not needed (thus unsupported) for heap_multi_insert() */ Assert(!(options & HEAP_INSERT_NO_LOGICAL)); + AssertHasSnapshotForToast(relation); + needwal = RelationNeedsWAL(relation); saveFreeSpace = RelationGetTargetPageFreeSpace(relation, HEAP_DEFAULT_FILLFACTOR); @@ -2694,6 +2715,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 @@ -3189,6 +3212,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 -- 2.39.5 (Apple Git-154)
