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: <CADzfLwU-OmxW3t3AoQo9=K7uq4G1yZ-txcetzW3jbcVxV_pJew@mail.gmail.com>
References: <202512151349.vlq3mpfniyk3@alvherre.pgsql> <11247.1767609087@localhost> <11558.1767609632@localhost> <141054.1767891540@localhost> <CADzfLwU-OmxW3t3AoQo9=K7uq4G1yZ-txcetzW3jbcVxV_pJew@mail.gmail.com>
Comments: In-reply-to Mihail Nikalayeu <mihailnikalayeu@gmail.com>
   message dated "Sat, 10 Jan 2026 20:37:06 +0300."
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Date: Mon, 12 Jan 2026 17:33:30 +0100
Message-ID: <137668.1768235610@localhost>
Archived-At: <https://www.postgresql.org/message-id/137668.1768235610%40localhost>
Precedence: bulk

--=-=-=
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Mihail Nikalayeu <mihailnikalayeu@gmail.com> wrote:

> On Thu, Jan 8, 2026 at 7:59=E2=80=AFPM Antonin Houska <ah@cybertec.at> wr=
ote:
> > v29 tries to fix the problem.
>=20
> Some comments for 0001-0004.

Thanks.

> ------ 0001 -----

> > * FIXME: this is missing a way to specify the index to use to repack one
> > * table, or whether to pass a WITH INDEX clause when multiple tables are
> > * used.  Something like --index[=3Dindexname].  Adding that bleeds into
> > * vacuuming.c as well.
>=20
> Comments look stale.

This is an open question, see [1].

> > return "???";
> I think it is better to add Assert(false); before (done that way in a
> few places).

This is not really uncommon, see for example event_trigger.c. Added the
comment

	/* keep compiler quiet */

> > =E2=80=9CAn utility=E2=80=9D
> Should be =E2=80=9CA utility=E2=80=9D

Not sure it *should be* [2], but "a utility" appears to be much more common=
 in
the tree. Changed.

> > else if (pg_strcasecmp(cmd, "CLUSTER") =3D=3D 0)
> >    cmdtype =3D PROGRESS_COMMAND_CLUSTER;
>=20
> Should we set PROGRESS_COMMAND_REPACK here? Because cluster is not
> used anywhere. Probably we may even delete PROGRESS_COMMAND_CLUSTER.

Good point. Actually we do not need this branch at all as there's no
pg_stat_get_progress_info('CLUSTER') call in system_views.sql. Removed.

> > CLUOPT_RECHECK_ISCLUSTERED
> It is not set anymore... Probably something is wrong here or we need
> to just remove that constant and check for it.

Yes, it got lost somehow. I added it where I think it's appropriate.

> ------ 0002 -----
>=20
> > rebuild_relation(Relation OldHeap, Relation index, bool verbose)
> It removes unused cmd parameter, but I think it is better to not add
> it in the previous commit.

Yes, the changes were not correctly split into diffs. Fixed.

> ------ 0003 -----
>=20
> > int       newxcnt =3D 0;
>=20
> I think it is better to use uint32 for consistency here.

This diff only moves code across functions, I'm not going to do other chang=
es
now.

> Also, I think it is worth adding Assert(snapshot->snapshot_type =3D=3D
> SNAPSHOT_HISTORIC_MVCC)

ok

> ------ 0004 -----
>=20
> > /* Is REPACK (CONCURRENTLY) being run by this backend? */
> > if (am_decoding_for_repack())
>=20
> We should check change_useless_for_repack here - to avoid looking and
> TRUNCATE of unrelated tables.

In v29, if XLOG_HEAP_TRUNCATE of an unrelated table is seen here, we'll rai=
se
ERROR unnecessarily instead of truncating the table. That's obviously wrong=
 as
well. On the other hand, it's not trivial to teach change_useless_for_repac=
k()
to filter the TRUNCATE records by file locator. So besides adding a call of
change_useless_for_repack(), I removed that ereport(ERROR) from heap_decode=
()
and added a comment to plugin_change() explaining why TRUNCATE on the table
being repacked should fire the Assert() statement: TRUNCATE shouldn't appear
here due to locking.

> > /* For the same reason, unlock TOAST relation. */
> > if (OldHeap->rd_rel->reltoastrelid)
> >    LockRelationOid(OldHeap->rd_rel->reltoastrelid, AccessExclusiveLock);
>=20
> Hm, we are locking here instead of unlocking ;)

Copy-pasto, the lock level is incorrect as well. Actually the whole idea of
unlocking index and TOAST relation is probably wrong: if some transaction
already locked the table with a lock that does not conflict with
ShareUpdateExclusiveLock, it should not need to wait for
ShareUpdateExclusiveLock on index / TOAST relation. At least I don't recall=
 a
case where index / TOAST requires stronger lock than the main table. So I
removed the unlocking altogether.

> > (errhint("Relation \"%s\" has no identity index.",
> >       RelationGetRelationName(rel)))));
>=20
> One level of braces may be removed.
> > * to decode on behalf of REPACK (CONCURRENT)?
>=20
> CONCURRENTLY
> > * If recheck is required, it must have been preformed on the source
>=20
> "performed"
> > * On exit,'*scan_p' contains the scan descriptor used. The caller must =
close
> > * it when he no longer needs the tuple returned.
>=20
> There is no scan_p argument here.
> > * Copyright (c) 2012-2025, PostgreSQL Global Development Group
>=20
> 2026

ok

> > newtuple =3D change->data.tp.newtuple !=3D NULL ?
> >   change->data.tp.newtuple : NULL;
>=20
> > oldtuple =3D change->data.tp.oldtuple !=3D NULL ?
> >    change->data.tp.oldtuple : NULL;
> > newtuple =3D change->data.tp.newtuple !=3D NULL ?
> >    change->data.tp.newtuple : NULL;
>=20
> Hm, should it be just x =3D y ?

Thanks for spotting this. The reason is that significant portion of this pa=
tch
is copied from the pg_squeeze extension, and there it originally looked lik=
e:

	oldtuple =3D change->data.tp.oldtuple !=3D NULL ?
		&change->data.tp.oldtuple->tuple : NULL;

I failed to notice the unnecessary complexity when adapting the code to the
commit 08e6344fd642 in postgres core, and then copied it to the REPACK
patch. Fixed now.

> > apply_concurrent_insert
>=20
> Double newline at function start.

ok

> > heap2_decode
>=20
> Should we check for change_useless_for_repack here also? (for multi
> insert, for example).

Yes, done, thanks. While doing that, I've done the same for XLOG_HEAP_CONFI=
RM
in heap_decode() as it'd be bad for reorderbuffer.c to receive the CONFIRM
record w/o previously receiving the actual (speculative) INSERT.


[1] https://www.postgresql.org/message-id/7224.1762326739%40localhost
[2] https://en.wiktionary.org/wiki/an#Usage_notes

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


--=-=-=
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment; filename=v30-0001-Add-REPACK-command.patch
Content-Transfer-Encoding: quoted-printable