Thread: FreezeLimit underflows in pg14 and 15 causing incorrect behavior in heap_prepare_freeze_tuple

While investigating a bug over on [1], I found that
vacuum_set_xid_limits() is calculating freezeLimit in an unsafe way on
at least Postgres 14 and 15.

    limit = *oldestXmin - freezemin;
    safeLimit = ReadNextTransactionId() - autovacuum_freeze_max_age;
    if (TransactionIdPrecedes(limit, safeLimit))
        limit = *oldestXmin;
    *freezeLimit = limit;

All of these are unsigned, so it doesn't work very nicely when
freezemin (based on autovacuum_freeze_min_age) is larger than
oldestXmin and autovacuum_freeze_max_age is bigger than the next
transaction ID -- which is pretty common right after initdb, for
example.

I noticed the effect of this because FreezeLimit is saved in the
LVRelState and passed to heap_prepare_freeze_tuple() as cutoff_xid,
which is used to guard against freezing tuples that shouldn't be
frozen.

I didn't propose a fix because I just want to make sure I'm not
missing something first.

- Melanie

[1] https://www.postgresql.org/message-id/CAAKRu_Y_NJzF4-8gzTTeaOuUL3CcGoXPjXcAHbTTygT8AyVqag%40mail.gmail.com

On Fri, Jun 21, 2024 at 4:22 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> While investigating a bug over on [1], I found that
> vacuum_set_xid_limits() is calculating freezeLimit in an unsafe way on
> at least Postgres 14 and 15.
>
>     limit = *oldestXmin - freezemin;
>     safeLimit = ReadNextTransactionId() - autovacuum_freeze_max_age;
>     if (TransactionIdPrecedes(limit, safeLimit))
>         limit = *oldestXmin;
>     *freezeLimit = limit;
>
> All of these are unsigned, so it doesn't work very nicely when
> freezemin (based on autovacuum_freeze_min_age) is larger than
> oldestXmin and autovacuum_freeze_max_age is bigger than the next
> transaction ID -- which is pretty common right after initdb, for
> example.
>
> I noticed the effect of this because FreezeLimit is saved in the
> LVRelState and passed to heap_prepare_freeze_tuple() as cutoff_xid,
> which is used to guard against freezing tuples that shouldn't be
> frozen.
>
> I didn't propose a fix because I just want to make sure I'm not
> missing something first.

Hmm. So perhaps this subtraction results in the desired behavior for
freeze limit -- but by using FreezeLimit as the cutoff_xid for
heap_prepare_freeze_tuple(), you can still end up considering freezing
tuples with xmax older than OldestXmin.

This results in erroring out with "cannot freeze committed xmax" on 16
and master but not erroring out like this in 14 and 15 for the same
tuple and cutoff values.

- Melanie

On Sat, Jun 22, 2024 at 10:43 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> Hmm. So perhaps this subtraction results in the desired behavior for
> freeze limit -- but by using FreezeLimit as the cutoff_xid for
> heap_prepare_freeze_tuple(), you can still end up considering freezing
> tuples with xmax older than OldestXmin.

Using a FreezeLimit > OldestXmin is just wrong. I don't think that
that even needs to be discussed.

> This results in erroring out with "cannot freeze committed xmax" on 16
> and master but not erroring out like this in 14 and 15 for the same
> tuple and cutoff values.

I don't follow. I thought that 16 and master don't have this
particular problem? Later versions don't use safeLimit as FreezeLimit
like this.

--
Peter Geoghegan

On Sat, Jun 22, 2024 at 10:53 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Sat, Jun 22, 2024 at 10:43 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > Hmm. So perhaps this subtraction results in the desired behavior for
> > freeze limit -- but by using FreezeLimit as the cutoff_xid for
> > heap_prepare_freeze_tuple(), you can still end up considering freezing
> > tuples with xmax older than OldestXmin.
>
> Using a FreezeLimit > OldestXmin is just wrong. I don't think that
> that even needs to be discussed.

Because it is an unsigned int that wraps around, FreezeLimit can
precede OldestXmin, but we can have a tuple whose xmax precedes
OldestXmin but does not precede FreezeLimit. So, the question is if it
is okay to freeze tuples whose xmax precedes OldestXmin but follows
FreezeLimit.

> > This results in erroring out with "cannot freeze committed xmax" on 16
> > and master but not erroring out like this in 14 and 15 for the same
> > tuple and cutoff values.
>
> I don't follow. I thought that 16 and master don't have this
> particular problem? Later versions don't use safeLimit as FreezeLimit
> like this.

Yes, 16 and master don't consider freezing a tuple with an xmax older
than OldestXmin because they use OldestXmin for cutoff_xid and that
errors out in heap_prepare_freeze_tuple(). 14 and 15 (and maybe
earlier, but I didn't check) use FreezeLimit so they do consider
freezing tuple with xmax older than OldestXmin.

- Melanie