On Wed, Nov 27, 2024 at 9:29 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> 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.
>
Can we dodge adding this push call if we restrict the length of the
origin name to some reasonable limit like 256 or 512 and avoid the
need of toast altogether?
--
With Regards,
Amit Kapila.
