Re: Adding REPACK [concurrently]

Mihail Nikalayeu <mihailnikalayeu@gmail.com> wrote:

> On Thu, Dec 4, 2025 at 6:43 PM Antonin Houska <ah@cybertec.at> wrote:
> > v26 attached here. It's been rebased and reflects most of the feedback.
>
> Some comments on 0001-0002:
> 1)
>
> > cluster_rel(stmt->command, rel, indexOid, params);
> cluster_rel closes relation, and after it is dereferenced a few lines after.
> Technically it may be correct, but feels a little bit strange.

ok, will be fixed in the next version (supposedly later today).

> 2)
>
> > if (vacopts->mode == MODE_VACUUM)
> I think for better compatibility it is better to handle new value in
> if - (vacopts->mode == MODE_REPACK) to keep old cases unchanged

I suppose you mean vacuuming.c. We're considering removal of pg_repackdb from
the patchset, so let's decide on this later.

> 3)
>
> > case T_RepackStmt:
> >    tag = CMDTAG_REPACK;
> >    break;
>
> should we use instead:
>
> case T_RepackStmt:
>     if (((RepackStmt *) parsetree)->command == REPACK_COMMAND_CLUSTER)
>        tag = CMDTAG_CLUSTER;
>     else
>        tag = CMDTAG_REPACK;
>     break;
>
> or delete CMDTAG_CLUSTER - since it not used anymore

LGTM, will include it in the next version.

> 4)
> "has been superceded by"
> typo

ok. (This may also be removed, as it's specific to pg_repackdb.)

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