Re: Reject incompatible options specified with VACUUM ONLY_DATABASE_STATS

> On Oct 25, 2026, at 00:01, Jim Jones <jim(dot)jones(at)uni-muenster(dot)de> wrote:
>
> Hi Chao
>
> On 22/10/2026 08:23, Chao Li wrote:
>> When ONLY_DATABASE_STATS is enabled, no other option except VERBOSE may be enabled. But the current implementation does not check all options, for example:
>> ```
>> evantest=# vacuum (ONLY_DATABASE_STATS, SKIP_LOCKED);
>> ERROR: ONLY_DATABASE_STATS cannot be specified with other VACUUM options
>> evantest=# vacuum (ONLY_DATABASE_STATS, PARALLEL 1);
>> VACUUM
>> ```
>
> Thanks for the patch!

Hi Jim,

Thank you very much for the review.

>
> A few observations:
>
> == error message ==
>
> Instead of creating the new only_database_stats_conflict flag you could
> simply add a check in the if (params.options &
> VACOPT_ONLY_DATABASE_STATS) block:
>
> @@ -378,15 +378,17 @@ ExecVacuum(ParseState *pstate, VacuumStmt
> *vacstmt, bool isTopLevel)
> ereport(ERROR,
>
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("ONLY_DATABASE_STATS
> cannot be specified with a list of tables")));
> - /* don't require people to turn off PROCESS_TOAST/MAIN
> explicitly */
> - if (params.options & ~(VACOPT_VACUUM |
> - VACOPT_VERBOSE |
> -
> VACOPT_PROCESS_MAIN |
> -
> VACOPT_PROCESS_TOAST |
> -
> VACOPT_ONLY_DATABASE_STATS))
> - ereport(ERROR,
> -
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> - errmsg("ONLY_DATABASE_STATS
> cannot be specified with other VACUUM options")));
> + /* only VERBOSE may be specified alongside
> ONLY_DATABASE_STATS */
> + foreach(lc, vacstmt->options)
> + {
> + DefElem *opt = lfirst_node(DefElem, lc);
> +
> + if (strcmp(opt->defname, "verbose") != 0 &&
> + strcmp(opt->defname,
> "only_database_stats") != 0)
> + ereport(ERROR,
> +
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +
> errmsg("ONLY_DATABASE_STATS cannot be specified with other VACUUM
> options")));
> + }
>

I think your suggestion expresses “only these two option names are allowed” in one place, which is good. However, I am also concerned that if a future option needs a similar check, following this pattern would require another loop.

When I wrote the code, I followed the “_given” pattern used in many places, such as publicationcmds.c and foreigncmds.c, which records whether an option is specified during parsing.

So, I’d leave the v1 implementation as it is and wait for more opinions.

> v2 attached implements this handling in ExecVacuum -- feel free to
> revert it if you disagree.
>
> == regression tests ==
>
> All error cases are handled in the same path, so theoretically only a
> few options (representative ones) would suffice. But I realise that if
> the code ever changes, we might end up with untested code. I'd be
> inclined to leave them as-is, but I thought I should bring it up. If
> it's supposed to cover all possible options, you could consider adding
> VACUUM (ONLY_DATABASE_STATS, VERBOSE false).

Added the test case in v3.

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/