Postgres Pro
Downloads
Home   >   mailing lists

Thread: Re: Remove remnants of "snapshot too old"

Re: Remove remnants of "snapshot too old"

From
Andres Freund
Date:
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

Re: Remove remnants of "snapshot too old"

From
Tom Lane
Date:
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

Re: Remove remnants of "snapshot too old"

From
Nathan Bossart
Date:
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