On Wed, Sep 25, 2024 at 01:05:26PM +0900, Michael Paquier wrote:
> On Tue, Sep 24, 2024 at 02:26:08PM -0500, Nathan Bossart wrote:
>> So... maybe we
>> should just remove pg_replication_origin's TOAST table instead...
>
> I'd rather keep it, FWIW. Contrary to pg_authid it does not imply
> problems at the same scale because we would have access to the toast
> relation in all the code paths with logical workers or table syncs.
> The other one was at early authentication stages.
Okay.
> It sounds to me that we should be much more proactive in detecting
> these failures and add something like that on HEAD. That's cheap
> enough. As the checks are the same for all these code paths, perhaps
> just hide them behind a local macro to reduce the duplication?
In v2, I moved the assertions to a new function called by the heapam.c
routines. I was hoping to move them to the tableam.h routines, but several
callers (in particular, the catalog/indexing.c ones that are causing
problems) call the heap ones directly. I've also included a 0001 patch
that introduces a RelationGetToastRelid() macro because I got tired of
typing "rel->rd_rel->reltoastrelid".
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?
--
nathan
>From 3eb1c0997e18d9e7a56cc3e974076e58613a1bb9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Tue, 8 Oct 2024 11:28:58 -0500
Subject: [PATCH v2 1/2] add RelationGetToastRelid macro
---
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 78e59384d1..966cf39f34 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -3075,7 +3075,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 6084dfa97c..7f07fda372 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 af8c05b91f..1788fbcf60 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2139,7 +2139,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,
@@ -15160,10 +15160,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);
@@ -15252,7 +15252,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 5f4f8053ee4e11276540d9cd48e149c8f02a7588 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Tue, 8 Oct 2024 13:39:30 -0500
Subject: [PATCH v2 2/2] ensure we have a snapshot if we might need toast
access
---
src/backend/access/heap/heapam.c | 25 +++++++++++++++++++++
src/backend/commands/tablecmds.c | 8 +++++++
src/backend/replication/logical/tablesync.c | 10 +++++++++
src/backend/replication/logical/worker.c | 8 +++++++
4 files changed, 51 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
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1788fbcf60..2e468d1d7d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -19281,9 +19281,17 @@ ATExecDetachPartition(List **wqueue, AlteredTableInfo
*tab, Relation rel,
tab->rel = rel;
}
+ 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..ed7d187bfe 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -377,6 +377,8 @@ process_syncing_tables_for_sync(XLogRecPtr current_lsn)
replorigin_session_origin_lsn = InvalidXLogRecPtr;
replorigin_session_origin_timestamp = 0;
+ PushActiveSnapshot(GetTransactionSnapshot());
+
/*
* Drop the tablesync's origin tracking if exists.
*
@@ -387,6 +389,8 @@ process_syncing_tables_for_sync(XLogRecPtr current_lsn)
*/
replorigin_drop_by_name(originname, true, false);
+ PopActiveSnapshot();
+
finish_sync_worker();
}
else
@@ -483,6 +487,8 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
started_tx = true;
}
+ PushActiveSnapshot(GetTransactionSnapshot());
+
/*
* Remove the tablesync origin tracking if
exists.
*
@@ -500,6 +506,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.
*/
@@ -1456,7 +1464,9 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
* logged for the purpose of recovery. Locks are to prevent the
* replication origin from vanishing while advancing.
*/
+ 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..502033f3d8 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -4604,8 +4604,10 @@ run_apply_worker()
walrcv_startstreaming(LogRepWorkerWalRcvConn, &options);
StartTransactionCommand();
+ PushActiveSnapshot(GetTransactionSnapshot());
UpdateTwoPhaseState(MySubscription->oid,
LOGICALREP_TWOPHASE_STATE_ENABLED);
MySubscription->twophasestate =
LOGICALREP_TWOPHASE_STATE_ENABLED;
+ PopActiveSnapshot();
CommitTransactionCommand();
}
else
@@ -4821,7 +4823,9 @@ DisableSubscriptionAndExit(void)
/* Disable the subscription */
StartTransactionCommand();
+ PushActiveSnapshot(GetTransactionSnapshot());
DisableSubscription(MySubscription->oid);
+ PopActiveSnapshot();
CommitTransactionCommand();
/* Ensure we remove no-longer-useful entry for worker's start time */
@@ -4925,6 +4929,8 @@ clear_subscription_skip_lsn(XLogRecPtr finish_lsn)
started_tx = true;
}
+ PushActiveSnapshot(GetTransactionSnapshot());
+
/*
* Protect subskiplsn of pg_subscription from being concurrently updated
* while clearing it.
@@ -4983,6 +4989,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)
