MIME-Version: 1.0
References: <202508301750.cbohxyy2pcce@alvherre.pgsql>
 <202509251758.t77p7r2ocmej@alvherre.pgsql>
In-Reply-To: <202509251758.t77p7r2ocmej@alvherre.pgsql>
From: Robert Treat <rob@xzilla.net>
Date: Fri, 26 Sep 2025 13:30:09 -0400
Message-ID: 
 <CAJSLCQ303dwCBWoJmp55-f6uXDaRZpozeE+hvBUWau1QqvD2_A@mail.gmail.com>
Subject: Re: Adding REPACK [concurrently]
To: =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>,
 Antonin Houska <ah@cybertec.at>,
	Fujii Masao <masao.fujii@gmail.com>,
 Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAJSLCQ303dwCBWoJmp55-f6uXDaRZpozeE%2BhvBUWau1QqvD2_A%40mail.gmail.com>
Precedence: bulk

On Thu, Sep 25, 2025 at 2:12=E2=80=AFPM =C3=81lvaro Herrera <alvherre@kuril=
emu.de> wrote:
> So here's v22 with those and rebased to current sources.  Only the first
> two patches this time, which are the ones I would be glad to receive
> input on.
>

A number of small issues I noticed. I don't know that they all need
addressing right now, but seems worth asking the questions...

#1
"pg_repackdb --help" does not mention the --index option, although the
flag is accepted. I'm not sure if this is meant to match clusterdb,
but since we need the index option to invoke the clustering behavior,
I think it needs to be there.

#2
[xzilla@zebes] pgsql/bin/pg_repackdb -d pagila -v -t customer
--index=3Didx_last_name
pg_repackdb: repacking database "pagila"
INFO:  clustering "public.customer" using sequential scan and sort

[xzilla@zebes] pgsql/bin/pg_repackdb -d pagila -v -t customer
pg_repackdb: repacking database "pagila"
INFO:  vacuuming "public.customer"

This was less confusing once I figured out we could pass the --index
option, but even with that it is a little confusing, I think mostly
because it looks like we are "vacuuming" the table, which in a world
of repack and vacuum (ie. no vacuum full) doesn't make sense. I think
the right thing to do here would be to modify it to be "repacking %s"
in both cases, with the "using sequential scan and sort" as the means
to understand which version of repack is being executed.

#3
pg_repackdb does not offer an --analyze option, which istm it should
to match the REPACK command

#4
SQL level REPACK help shows:

where option can be one of:
    VERBOSE [ boolean ]
    ANALYSE | ANALYZE

but SQL level VACUUM does
    VERBOSE [ boolean ]
    ANALYZE [ boolean ]

These operate the same way, so I would expect it to match the language
in vacuum.

#5
[xzilla@zebes] pgsql/bin/pg_repackdb -d pagila -v -t film --index
pg_repackdb: repacking database "pagila"

In the above scenario, I am repacking without having previously
specified an index. At the SQL level this would throw an error, at the
command line it gives me a heart attack. :-)
It's actually not that bad, because we don't actually do anything, but
maybe we should throw an error?

#6
On the individual command pages (like sql-repack.html), I think there
should be more cross-linking, ie. repack should probably say "see also
cluster" and vice versa. Likely similarly with vacuum and repack.

#7
Is there some reason you chose to intermingle the repack regression
tests with the existing tests? I feel like it'd be easier to
differentiate potential regressions and new functionality if these
were separated.


Robert Treat
https://xzilla.net