Re: Remove remnants of "snapshot too old"

On Tue, Dec 03, 2024 at 10:06:59PM +0200, Heikki Linnakangas wrote:
> -/* ----------
> - * init_toast_snapshot
> - *
> - * Initialize an appropriate TOAST snapshot. We must use an MVCC snapshot
> - * to initialize the TOAST snapshot; since we don't know which one to use,
> - * just use the oldest one.
> - */
> -void
> -init_toast_snapshot(Snapshot toast_snapshot)
> -{
> - Snapshot snapshot = GetOldestSnapshot();
> -
> - /*
> - * GetOldestSnapshot returns NULL if the session has no active snapshots.
> - * We can get that if, for example, a procedure fetches a toasted value
> - * into a local variable, commits, and then tries to detoast the value.
> - * Such coding is unsafe, because once we commit there is nothing to
> - * prevent the toast data from being deleted. Detoasting *must* happen in
> - * the same transaction that originally fetched the toast pointer. Hence,
> - * rather than trying to band-aid over the problem, throw an error. (This
> - * is not very much protection, because in many scenarios the procedure
> - * would have already created a new transaction snapshot, preventing us
> - * from detecting the problem. But it's better than nothing, and for sure
> - * we shouldn't expend code on masking the problem more.)
> - */
> - if (snapshot == NULL)
> - elog(ERROR, "cannot fetch toast data without an active snapshot");
> -
> - /*
> - * Catalog snapshots can be returned by GetOldestSnapshot() even if not
> - * registered or active. That easily hides bugs around not having a
> - * snapshot set up - most of the time there is a valid catalog snapshot.
> - * So additionally insist that the current snapshot is registered or
> - * active.
> - */
> - Assert(HaveRegisteredOrActiveSnapshot());
> -
> - InitToastSnapshot(*toast_snapshot, snapshot->lsn, snapshot->whenTaken);
> -}

The ERROR and Assert() here were the subject of some recent commits
(b52adba and 70b9adb) as well as a patch I'm hoping to commit soon [0].
I've only skimmed your patch so far, but I'm wondering if this fixes those
issues a different way. If not, I'm a little worried about losing these
checks.

[0] https://postgr.es/m/flat/ZyKdCEaUB2lB1rG8%40nathan

--
nathan