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
