Re: splitting src/bin/scripts/vacuumdb.c

Álvaro Herrera <alvherre(at)kurilemu(dot)de> wrote:

> - Two booleans, analyze_in_stages and analyze_only, are really
> determining what "mode" the program runs in -- it's either vacuum, or
> one of those two modes. (Antonin came up with the idea of using
> "modes", but his patch only adds a new REPACK mode on top of the
> existing code without any further changes). I think the code flow is a
> little neater by making this change; consider for instance this change:
>
> - /* If we used SKIP_DATABASE_STATS, mop up with ONLY_DATABASE_STATS */
> - if (vacopts->skip_database_stats && stage == ANALYZE_NO_STAGE &&
> - !vacopts->analyze_only)
> - {
>
> + /* If we used SKIP_DATABASE_STATS, mop up with ONLY_DATABASE_STATS */
> + if (vacopts->mode == MODE_VACUUM && vacopts->skip_database_stats)
> + {
>
> IMO the original is quite unclear.

I agree that redundant information makes things more difficult to think
about. I just wonder if

vacopts->mode != MODE_VACUUM

should be used instead of

(vacopts->mode == MODE_ANALYZE ||
vacopts->mode == MODE_ANALYZE_IN_STAGES)

in some places. Nevertheless, I'm not proposing a global replacement: the
verbose form may be easier to understand, depending on the context.

Other than that, I checked differences between v21, v22 and v23. I've got no
other comments worth posting.

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