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]/messages/by-id/flat/ZyKdCEaUB2lB1rG8@nathan.
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]: /messages/by-id/flat/ZyKdCEaUB2lB1rG8@nathan

--
nathan

Hi,

On 2024-12-03 22:06:59 +0200, Heikki Linnakangas wrote:

I spotted some more remnants of the "snapshot too old" feature that was
removed in v17. Barring objections, I will commit the attached patch to tidy
up.

Most of this I agree with. But I'm not sure just removing the toast snapshot
stuff is good - we've had a bunch of bugs where we don't hold a snapshot for
long enough to actually ensure that toast tuples stay alive. It's not legal to
fetch a toast id in one snapshot, release that, and then fetch the toast tuple
with a fresh snapshot. I think the removal of

- if (snapshot == NULL)
- elog(ERROR, "cannot fetch toast data without an active snapshot");

will make it easier to introduce further such bugs?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2024-12-03 22:06:59 +0200, Heikki Linnakangas wrote:

I spotted some more remnants of the "snapshot too old" feature that was
removed in v17. Barring objections, I will commit the attached patch to tidy
up.

Most of this I agree with. But I'm not sure just removing the toast snapshot
stuff is good - we've had a bunch of bugs where we don't hold a snapshot for
long enough to actually ensure that toast tuples stay alive.

Yeah, the stuff concerned with toast snapshots has nothing to do
with that old user-visible feature. It's to keep us from writing
incorrect code, and it's still (very) needed.

regards, tom lane

On 04/12/2024 03:24, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2024-12-03 22:06:59 +0200, Heikki Linnakangas wrote:

I spotted some more remnants of the "snapshot too old" feature that was
removed in v17. Barring objections, I will commit the attached patch to tidy
up.

Most of this I agree with. But I'm not sure just removing the toast snapshot
stuff is good - we've had a bunch of bugs where we don't hold a snapshot for
long enough to actually ensure that toast tuples stay alive.

Yeah, the stuff concerned with toast snapshots has nothing to do
with that old user-visible feature. It's to keep us from writing
incorrect code, and it's still (very) needed.

Right. Here's a new attempt that keeps that check.

--
Heikki Linnakangas
Neon (https://neon.tech)

On Wed, Dec 04, 2024 at 09:55:43AM +0200, Heikki Linnakangas wrote:

On 04/12/2024 03:24, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Most of this I agree with. But I'm not sure just removing the toast snapshot
stuff is good - we've had a bunch of bugs where we don't hold a snapshot for
long enough to actually ensure that toast tuples stay alive.

Yeah, the stuff concerned with toast snapshots has nothing to do
with that old user-visible feature. It's to keep us from writing
incorrect code, and it's still (very) needed.

Right. Here's a new attempt that keeps that check.

Looks reasonable to me. One idea I had is to make SnapshotToast a macro
that first does an AssertMacro(HaveRegisteredOrActiveSnapshot()), but the
approach in the patch seems fine, too.

--
nathan