Re: Large expressions in indexes can't be stored (non-TOASTable)

On Tue, Sep 24, 2024 at 02:26:08PM -0500, Nathan Bossart wrote:
> 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.

Regression tests don't blow up after this patch and the reindex parts.

> 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.

Ahh.  That's the tablecmds.c part for the partition detach.

> 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...

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.

+	/*
+	 * If we might need TOAST access, make sure the caller has set up a valid
+	 * snapshot.
+	 */
+	Assert(HaveRegisteredOrActiveSnapshot() ||
+		   !OidIsValid(heapRel->rd_rel->reltoastrelid) ||
+		   !IsNormalProcessingMode());
+

I didn't catch that we could just reuse the opened Relation in these
paths and check for reltoastrelid.  Nice.

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?

Not the responsibility of this patch, but the business with
clear_subscription_skip_lsn() with its conditional transaction start
feels messy.  This comes down to the way handles work for 2PC and the
streaming, which may or may not be in a transaction depending on the
state of the upper caller.  Your patch looks right in the way
snapshots are set, as far as I've checked.
--
Michael