Re: vacuumdb: add --dry-run
| От | Chao Li |
|---|---|
| Тема | Re: vacuumdb: add --dry-run |
| Дата | |
| Msg-id | 6460D2FC-A909-4C21-A1AE-6C94A22060ED@gmail.com обсуждение исходный текст |
| Ответ на | Re: vacuumdb: add --dry-run (Nathan Bossart <nathandbossart@gmail.com>) |
| Список | pgsql-hackers |
Hi Nathan,
I just reviewed v5, and overall looks very good patch quality. Just a few nit comments on 0001 and 0003.
> On Dec 5, 2025, at 05:52, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
>
> --
> nathan
>
<v5-0001-Move-some-vacuumdb-options-to-vacopts-struct.patch><v5-0002-Add-ParallelSlotSetIdle.patch><v5-0003-Add-dry-run-to-vacuumdb.patch>
1 - 0001
```
/* initialize options */
memset(&vacopts, 0, sizeof(vacopts));
vacopts.objfilter = 0; /* no filter */
vacopts.parallel_workers = -1;
vacopts.buffer_usage_limit = NULL;
vacopts.no_index_cleanup = false;
vacopts.force_index_cleanup = false;
vacopts.do_truncate = true;
vacopts.process_main = true;
vacopts.process_toast = true;
```
Now echo and print are moved into vacopts and their default values are false. Here, memset() have properly initialized
theirvalues. But this piece of code still explicitly set boolean values to vacopts fields. So, to make it consistent, I
feelwe can also add explicit assignments to echo and print here, or remove those “false” assignments. This is not a
correctnessissue, just to keep in a consistent style.
2 - 0003
```
+ if (echo || dry_run)
+ printf("%s%s\n", sql, dry_run ? " -- not executed" : "");
```
There are two white-spaces before “--“, I think one is enough, In the other place of 0003, you just one white-space
before“--“.
3 - 0003
```
@@ -1001,15 +1009,19 @@ prepare_vacuum_command(PGconn *conn, PQExpBuffer sql,
* Any errors during command execution are reported to stderr.
*/
static void
-run_vacuum_command(PGconn *conn, const char *sql, bool echo,
- const char *table)
+run_vacuum_command(ParallelSlot *free_slot, const char *sql,
+ bool echo, bool dry_run, const char *table)
{
```
Here are two comments:
* As run_vacuum_command() takes both echo and dry_run, and both of them are defined in vcaopts, why not change this
functionto 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.
4 - 0003
```
+ if (vacopts.dry_run)
+ pg_log_info("Executing in dry-run mode.”);
```
Feels like “Running” is better than “Executing”.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
В списке pgsql-hackers по дате отправления: