On Sat, Dec 06, 2025 at 07:48:22AM +0800, Chao Li wrote:
> I just reviewed v5, and overall looks very good patch quality. Just a few
> nit comments on 0001 and 0003.
I've attached an updated patch set. After giving the "dry-run" messages
another look, I think we should just print a note at the very beginning of
vacuumdb execution and leave it at that. The per-database messages weren't
translator friendly and IMHO didn't add much, and the "-- not executed"
comments were noisy and didn't reflect the commands that would've been sent
to the server.
> Now echo and print are moved into vacopts and their default values are
> false. Here, memset() have properly initialized their values. But this
> piece of code still explicitly set boolean values to vacopts fields. So,
> to make it consistent, I feel we can also add explicit assignments to
> echo and print here, or remove those “false” assignments. This is not a
> correctness issue, just to keep in a consistent style.
We are already pretty inconsistent about this. If anything, I think we
should do the opposite, i.e., remove any unnecessary initializations to
0/false/NULL. The memset() makes those redundant and should suffice in
most cases.
> * As run_vacuum_command() takes both echo and dry_run, and both of them
> are defined in vcaopts, why not change this function to take a const
> vcaopts * instead of two bools?
>
> * The function comment needs to be updated. Now it won’t always send a
> command to server, with “dry_run”, it behaves differently.
Done.
> ```
> + if (vacopts.dry_run)
> + pg_log_info("Executing in dry-run mode.”);
> ```
>
> Feels like “Running” is better than “Executing”.
Done.
--
nathan
