MIME-Version: 1.0
References: <202507262156.sb455angijk6@alvherre.pgsql>
 <202510301734.pj4uds3mqxx4@alvherre.pgsql>
 <CADzfLwWdc1KBZ2qNV1x7gmZtHdmAYOoq0A2Rw72O2-wEou=FRg@mail.gmail.com>
 <11472.1762156600@localhost>
In-Reply-To: <11472.1762156600@localhost>
From: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Date: Tue, 2 Dec 2025 01:50:00 +0100
Message-ID: 
 <CADzfLwVtct0_GbtKt8hksR6rX2ozEtKSS4b=YmEOPghHkEE6XQ@mail.gmail.com>
Subject: Re: Adding REPACK [concurrently]
To: Antonin Houska <ah@cybertec.at>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Pg Hackers <pgsql-hackers@lists.postgresql.org>,
 Robert Treat <rob@xzilla.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CADzfLwVtct0_GbtKt8hksR6rX2ozEtKSS4b%3DYmEOPghHkEE6XQ%40mail.gmail.com>
Precedence: bulk

Hello, Antonin!

On Mon, Nov 3, 2025 at 8:56=E2=80=AFAM Antonin Houska <ah@cybertec.at> wrot=
e:
> I'll fix all the problems in the next version. Thanks!

A few more moments I mentioned:

> switch ((vis =3D HeapTupleSatisfiesVacuum(tuple, OldestXmin, buf)))
vis is unused, also to double braces.

>       LockBuffer(buf, BUFFER_LOCK_UNLOCK);
>       continue;
>    }

>    /*
>     * In the concurrent case, we have a copy of the tuple, so we
>     * don't worry whether the source tuple will be deleted / updated
>     * after we release the lock.
>     */
>    LockBuffer(buf, BUFFER_LOCK_UNLOCK);
>}

I think locking and comments are a little bit confusing here.
I think we may use single LockBuffer(buf, BUFFER_LOCK_UNLOCK); before
`if (isdead)` as it was before.
Also, I am not sure "we have a copy" is the correct point here, I
think motivation is mostly the same as in
heapam_index_build_range_scan.

Also, I think it is a good idea to add tests for index-based and
sort-based repack.

Also, for sort-based I think we need to also call
repack_decode_concurrent_changes during insertion phase

> is_system_catalog && !concurrent
2 places, always true, feels strange.

Best regards,
Mikhail.