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

Mihail Nikalayeu <mihailnikalayeu@gmail.com> wrote:

> Hello, Antonin!
>=20
> On Mon, Nov 3, 2025 at 8:56=E2=80=AFAM Antonin Houska <ah@cybertec.at> wr=
ote:
> > I'll fix all the problems in the next version. Thanks!
>=20
> A few more moments I mentioned:
>=20
> > switch ((vis =3D HeapTupleSatisfiesVacuum(tuple, OldestXmin, buf)))
> vis is unused, also to double braces.
>=20
> >       LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> >       continue;
> >    }
>=20
> >    /*
> >     * 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);
> >}
>=20
> 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.

All these problems are due to incorrect separation of the "preserve
visibility" part of the patch series. Will be fixed in the next version.

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

Not sure, cluster.sql already seems to do the same.

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

I miss the point. The current coding is such that this part

	if (concurrent)
	{
		XLogRecPtr	end_of_wal;

		end_of_wal =3D GetFlushRecPtr(NULL);
		if ((end_of_wal - end_of_wal_prev) > wal_segment_size)
		{
			repack_decode_concurrent_changes(decoding_ctx, end_of_wal);
			end_of_wal_prev =3D end_of_wal;
		}
	}

gets called regardless the value of 'tuplesort' above.

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

ok

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