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

Álvaro Herrera <alvherre@kurilemu.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