Horton

Re: BUG: Postgres 14 + vacuum_defer_cleanup_age + FOR UPDATE + UPDATE

Matthias van de Meent <boekewurm+postgres@gmail.com>

View thread View on postgr.es
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Michail Nikolaev <michail.nikolaev@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, Peter Geoghegan <pg@bowt.ie>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Robert Haas <robertmhaas@gmail.com>, Mark Dilger <mark.dilger@enterprisedb.com>
Date: 2023-01-10T14:03:42Z
Lists: pgsql-hackers 
On Mon, 9 Jan 2023 at 20:34, Andres Freund <andres@anarazel.de> wrote:
> On 2023-01-09 17:50:10 +0100, Matthias van de Meent wrote:
> > Wouldn't it be enough to only fix the constructions in
> > FullXidRelativeTo() and widen_snapshot_xid() (as attached, $topic does
> > not occur with the patch), and (optionally) bump the first XID
> > available for any cluster to (FirstNormalXid + 1) to retain the 'older
> > than any running transaction' property?
>
> It's not too hard to fix in individual places, but I suspect that we'll
> introduce the bug in future places without some more fundamental protection.
>
> Locally I fixed it by clamping vacuum_defer_cleanup_age to a reasonable value
> in ComputeXidHorizons() and GetSnapshotData().

I don't think that clamping the value with oldestXid (as seen in patch
0001, in GetSnapshotData) is right.
It would clamp the value relative to the oldest frozen xid of all
databases, which can be millions of transactions behind oldestXmin,
and thus severely skew the amount of transaction's changes you keep on
disk (that is, until oldestXid moves past 1000_000).
A similar case can be made for the changes in ComputeXidHorizons - for
the described behaviour of vacuum_defer_cleanup_age we would need to
clamp the used offset separately for each of the fields in the horizon
result to retain all transaction data for the first 1 million
transactions, and the ones that may still see these transactions.

> Fixing it in FullXidRelativeTo() doesn't seem quite right, while it's ok to
> just return FirstNormalTransactionId in the case of vacuum_defer_cleanup_age,
> it doesn't see necessarily correct for other cases.

Understood.

Kind regards,

Matthias van de Meent

Commits

  1. Fix incorrect TAP test ordering

  2. pg_amcheck: Minor test speedups

  3. amcheck: Fix FullTransactionIdFromXidAndCtx() for xids before epoch 0

  4. amcheck: Fix ordering bug in update_cached_xid_range()

  5. Fix corruption due to vacuum_defer_cleanup_age underflowing 64bit xids

  6. Add hardening to catch invalid TIDs in indexes.

  7. Use full 64-bit XID for checking if a deleted GiST page is old enough.

  8. Avoid early reuse of btree pages, causing incorrect query results.