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
