From: Antonin Houska <ah@cybertec.at>
To: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
    Fujii Masao <masao.fujii@gmail.com>, Robert Treat <rob@xzilla.net>,
    Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Adding REPACK [concurrently]
In-reply-to: 
 <CADzfLwUYc+9X44r=kf8RmivD_z4xs0MyoZBiE-TsBZrppq64sA@mail.gmail.com>
References: 
 <CADzfLwXx46j8KwQjjM1ZcqNBsx-k6GxHOzDJkm4SHjh+cv31Rw@mail.gmail.com>
 <202508091333.qvgvo7ikuezm@alvherre.pgsql>
 <CADzfLwW=b=U3e6aasi=XorN8hZSiCKZErKs9qhyK7m=w=wokAg@mail.gmail.com>
 <40729.1755799624@localhost>
 <CADzfLwXpyzGGVB+nbSAMBWDBhzTyn6F2hRrAqcyJd3c6gT2EOQ@mail.gmail.com>
 <9536.1756127358@localhost>
 <CADzfLwWdw=5KhXE5Fr3khytp4mW2girFVJHHb4jSgEncc6237Q@mail.gmail.com>
 <21931.1756136535@localhost>
 <CADzfLwWPihwPM=e=EJ_U5niqDdVyq9pS30ZNxwZ8f_yXyikooQ@mail.gmail.com>
 <24483.1756142534@localhost>
 <CADzfLwUqyOmpkLmciecBy4aBN1sohQVZ2Hgc6m-tjSUqDRHwyQ@mail.gmail.com>
 <4790.1756197960@localhost>
 <CADzfLwV+80MfPM=MC5y3nA34djUWuYU6YKcZUO8JjD7_8p7nkg@mail.gmail.com>
 <29527.1756215093@localhost>
 <CADzfLwW3XAFKYkBY7Jktf_rETzcqMj20GXgjESqu+WEoeEMkog@mail.gmail.com>
 <6931.1756275367@localhost>
 <CADzfLwUYc+9X44r=kf8RmivD_z4xs0MyoZBiE-TsBZrppq64sA@mail.gmail.com>
Comments: In-reply-to Mihail Nikalayeu <mihailnikalayeu@gmail.com>
   message dated "Wed, 27 Aug 2025 10:22:24 +0200."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <17669.1756289505.1@localhost>
Date: Wed, 27 Aug 2025 12:11:45 +0200
Message-ID: <17670.1756289505@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/17670.1756289505%40localhost>
Precedence: bulk

Mihail Nikalayeu <mihailnikalayeu@gmail.com> wrote:

> > A new kind of snapshot seems like (much) cleaner solution at the moment.
> 
> Do you mean some kind of snapshot which only uses
> TransactionIdDidCommit/Abort ignoring
> TransactionIdIsCurrentTransactionId/TransactionIdIsInProgress?
> Actually it behaves like SnapshotBelieveEverythingCommitted in that
> particular case, but TransactionIdDidCommit/Abort may be used as some
> kind of assert/error source to be sure everything is going as
> designed.

Given that there should be no (sub)transaction aborts in the new table, I
think you only need to check that the tuple has valid xmin and invalid xmax.

I think the XID should be in the commit log at the time the transaction is
being replayed, so it should be legal to use TransactionIdDidCommit/Abort in
Assert() statements. (And as long as REPACK CONCURRENTLY will use
ShareUpdateExclusiveLock, which conflicts with VACUUM, pg_class(relfrozenxid)
for given table should not advance during the processing, and therefore the
replayed XIDs should not be truncated from the commit log while REPACK
CONCURRENTLY is running.)

> And, yes, for the new snapshot we need to have
> HeapTupleSatisfiesUpdate to be modified.
> 
> Also, to deal with that particular race we may just use
> XactLockTableWait(xid, NULL, NULL, XLTW_None) before starting
> transaction replay.

Do you mean the race related to TransactionIdIsInProgress()? Not sure I
understand, as you suggested above that you no longer need the function.

> > No rush. First, the MVCC safety is not likely to be included in v19 [1].
> 
> That worries me - it is not the behaviour someone expects from a
> database by default. At least the warning should be much more visible
> and obvious.
> I think most of user will expect the same guarantees as [CREATE|RE]
> INDEX CONCURRENTLY provides.

It does not really worry me. The pg_squeeze extension is not MVCC-safe and I
remember there were only 1 or 2 related complaints throughout its
existence. (pg_repack isn't MVCC-safe as well, but I don't keep track of its
issues.)

Of course, user documentation should warn about the problem, in a way it does
for other commands (typically ALTER TABLE).

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com