From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alexander Lakhin <exclusion@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jonathan S. Katz <jkatz@postgresql.org>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Large expressions in indexes can't be stored (non-TOASTable)
Date: Tue, 24 Sep 2024 14:26:08 -0500
Message-ID: <ZvMSUPOqUU-VNADN@nathan> (raw)
In-Reply-To: <ZvI-WXcaFXuj1Dvp@paquier.xyz>
References: <ZuroEfWjxd4Zj6Yq@nathan>
<1812111.1726671296@sss.pgh.pa.us>
<ZusvlZBrPKm913LA@nathan>
<a97d7401-e7c9-f771-6a00-037379f0a8bb@gmail.com>
<ZuxvNDVfqpahqZNb@nathan>
<adb7421f-eaf4-878a-6742-576fc6bf0d8f@gmail.com>
<Zu2oJtMI2avvjmtl@nathan>
<e012fb69-ca34-7e0e-3a33-b16d9737e526@gmail.com>
<ZvGOPdGAronxOfmf@nathan>
<ZvI-WXcaFXuj1Dvp@paquier.xyz>
On Tue, Sep 24, 2024 at 01:21:45PM +0900, Michael Paquier wrote:
> On Mon, Sep 23, 2024 at 10:50:21AM -0500, Nathan Bossart wrote:
>> I carefully inspected all the code paths this patch touches, and I think
>> I've got all the details right, but I would be grateful if someone else
>> could take a look.
>
> No objections from here with putting the snapshots pops and pushes
> outside the inner routines of reindex/drop concurrently, meaning that
> ReindexRelationConcurrently(), DefineIndex() and index_drop() are fine
> to do these operations.
Great. I plan to push 0001 shortly.
> Actually, thinking more... Could it be better to have some more
> sanity checks in the stack outside the toast code for catalogs with
> toast tables? For example, I could imagine adding a check in
> CatalogTupleUpdate() so as all catalog accessed that have a toast
> relation require an active snapshot. That would make checks more
> aggressive, because we would not need any toast data in a catalog to
> make sure that there is a snapshot set. This strikes me as something
> we could do better to improve the detection of failures like the one
> reported by Alexander when updating catalog tuples as this can be
> triggered each time we do a CatalogTupleUpdate() when dirtying a
> catalog tuple. The idea is then to have something before the
> HaveRegisteredOrActiveSnapshot() in the toast internals, for catalogs,
> and we would not require toast data to detect problems.
I gave this a try and, unsurprisingly, found a bunch of other problems. I
hastily hacked together the attached patch that should fix all of them, but
I'd still like to comb through the code a bit more. The three catalogs
with problems are pg_replication_origin, pg_subscription, and
pg_constraint. pg_contraint has had a TOAST table for a while, and I don't
think it's unheard of for conbin to be large, so this one is probably worth
fixing. pg_subscription hasn't had its TOAST table for quite as long, but
presumably subpublications could be large enough to require out-of-line
storage. pg_replication_origin, however, only has one varlena column:
roname. Three out of the seven problem areas involve
pg_replication_origin, but AFAICT that'd only ever be a problem if the name
of your replication origin requires out-of-line storage. So... maybe we
should just remove pg_replication_origin's TOAST table instead...
--
nathan
Attachments:toast_snapshot.patch (text/plain, 6.8K) downloaddiff --git a/src/backend/catalog/indexing.c b/src/backend/catalog/indexing.cindex d0d1abda58..b8be124555 100644--- a/src/backend/catalog/indexing.c+++ b/src/backend/catalog/indexing.c@@ -22,6 +22,7 @@
#include "catalog/index.h"
#include "catalog/indexing.h"
#include "executor/executor.h"
+#include "miscadmin.h"
#include "utils/rel.h"
@@ -234,6 +235,14 @@ CatalogTupleInsert(Relation heapRel, HeapTuple tup)
{
CatalogIndexState indstate;
+ /*+ * If we might need TOAST access, make sure the caller has set up a valid+ * snapshot.+ */+ Assert(HaveRegisteredOrActiveSnapshot() ||+ !OidIsValid(heapRel->rd_rel->reltoastrelid) ||+ !IsNormalProcessingMode());+
CatalogTupleCheckConstraints(heapRel, tup);
indstate = CatalogOpenIndexes(heapRel);
@@ -256,6 +265,14 @@ void
CatalogTupleInsertWithInfo(Relation heapRel, HeapTuple tup,
CatalogIndexState indstate)
{
+ /*+ * If we might need TOAST access, make sure the caller has set up a valid+ * snapshot.+ */+ Assert(HaveRegisteredOrActiveSnapshot() ||+ !OidIsValid(heapRel->rd_rel->reltoastrelid) ||+ !IsNormalProcessingMode());+
CatalogTupleCheckConstraints(heapRel, tup);
simple_heap_insert(heapRel, tup);
@@ -273,6 +290,14 @@ void
CatalogTuplesMultiInsertWithInfo(Relation heapRel, TupleTableSlot **slot,
int ntuples, CatalogIndexState indstate)
{
+ /*+ * If we might need TOAST access, make sure the caller has set up a valid+ * snapshot.+ */+ Assert(HaveRegisteredOrActiveSnapshot() ||+ !OidIsValid(heapRel->rd_rel->reltoastrelid) ||+ !IsNormalProcessingMode());+
/* Nothing to do */
if (ntuples <= 0)
return;
@@ -315,6 +340,14 @@ CatalogTupleUpdate(Relation heapRel, ItemPointer otid, HeapTuple tup)
CatalogIndexState indstate;
TU_UpdateIndexes updateIndexes = TU_All;
+ /*+ * If we might need TOAST access, make sure the caller has set up a valid+ * snapshot.+ */+ Assert(HaveRegisteredOrActiveSnapshot() ||+ !OidIsValid(heapRel->rd_rel->reltoastrelid) ||+ !IsNormalProcessingMode());+
CatalogTupleCheckConstraints(heapRel, tup);
indstate = CatalogOpenIndexes(heapRel);
@@ -339,6 +372,14 @@ CatalogTupleUpdateWithInfo(Relation heapRel, ItemPointer otid, HeapTuple tup,
{
TU_UpdateIndexes updateIndexes = TU_All;
+ /*+ * If we might need TOAST access, make sure the caller has set up a valid+ * snapshot.+ */+ Assert(HaveRegisteredOrActiveSnapshot() ||+ !OidIsValid(heapRel->rd_rel->reltoastrelid) ||+ !IsNormalProcessingMode());+
CatalogTupleCheckConstraints(heapRel, tup);
simple_heap_update(heapRel, otid, tup, &updateIndexes);
@@ -364,5 +405,13 @@ CatalogTupleUpdateWithInfo(Relation heapRel, ItemPointer otid, HeapTuple tup,
void
CatalogTupleDelete(Relation heapRel, ItemPointer tid)
{
+ /*+ * If we might need TOAST access, make sure the caller has set up a valid+ * snapshot.+ */+ Assert(HaveRegisteredOrActiveSnapshot() ||+ !OidIsValid(heapRel->rd_rel->reltoastrelid) ||+ !IsNormalProcessingMode());+
simple_heap_delete(heapRel, tid);
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.cindex d27e6cf345..06ac1d39d2 100644--- a/src/backend/commands/tablecmds.c+++ b/src/backend/commands/tablecmds.c@@ -19292,9 +19292,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.cindex 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.cindex 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();
}
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, michael@paquier.xyz, exclusion@gmail.com, tgl@sss.pgh.pa.us, jkatz@postgresql.org, pgsql-hackers@lists.postgresql.org
Subject: Re: Large expressions in indexes can't be stored (non-TOASTable)
In-Reply-To: <ZvMSUPOqUU-VNADN@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox