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

On Wed, Nov 27, 2024 at 03:20:19PM +0900, Michael Paquier wrote:
> On Mon, Nov 25, 2024 at 01:29:31PM -0600, Nathan Bossart wrote:
>> Or we could just enforce that you have an active snapshot whenever you
>> modify a catalog with a TOAST table.  That's simpler, but it requires extra
>> work in some paths (and probably comments to point out that we're only
>> pushing an active snapshot to satisfy an assertion).
> 
> I may be wrong, but I suspect that enforcing the check without being
> column-based is the right way to go and that this is going to catch
> more errors in the long-term than being a maintenance burden.  So I
> would keep the snapshot check even if it's a bit aggressive, still
> it's useful.  And we are not talking about that may code paths that
> need to be switched to require a snapshot, as well.  Most of the ones
> you have mentioned on this thread are really particular in the ways
> they do transaction handling.  I suspect that it may also catch
> out-of-core issues with extensions doing direct catalog manipulations.

That is useful feedback, thanks.

One other thing that caught my eye is that replorigin_create() uses
SnapshotDirty, so I'm unsure if
PushActiveSnapshot(GetTransactionSnapshot()) is correct there.  The only
other example I found is RelationFindReplTupleByIndex(), which uses
GetLatestSnapshot().  But I do see that CreateSubscription() calls
replorigin_create(), and it seems to rely on the transaction snapshot, so
maybe it's okay, at least for the purpose of TOAST table access...  I'm
finding myself wishing there was a bit more commentary about the proper
usage of these snapshot functions.  Maybe I can try to add some as a
follow-up exercise.

-- 
nathan