From 90e55052c176831a30b49f1def3c5b3d459b20fc Mon Sep 17 00:00:00 2001 From: Baji Shaik Date: Wed, 7 Oct 2026 09:52:54 -0500 Subject: [PATCH] Centralize last-wins handling of duplicate utility-command options Utility commands that accept a parenthesized option list (VACUUM, ANALYZE, EXPLAIN, CHECKPOINT, REPACK) each run their own loop over the DefElem list and intend "last specification wins" when an option is repeated. Implementing that per command is error-prone: a loop that OR'd a boolean, for example, would leave an option enabled even when its final specification was OFF (fixed for REPACK in commit a120ecf5498). Add a small shared helper, deduplicateDefElemList(), which returns the input option list reduced to the last occurrence of each option name, preserving order. Each command calls it once before its existing parse loop, so repeated options are collapsed uniformly and the "any-ON-wins" class of bug cannot recur. Per-command semantic checks (valid option names, value coercion, range checks, the VACUUM/ANALYZE split) are unchanged and stay in each command. This implements the DefElem-list-handling unification suggested by Thom Brown at https://www.postgresql.org/message-id/CAA-aLv57ymRoDJ%2B2UnHCE7WDB_FH5pW9%2BbFX7z5wEegudPrdRQ%40mail.gmail.com Author: Baji Shaik --- src/backend/commands/define.c | 55 +++++++++++++++++++++++++++ src/backend/commands/explain_state.c | 2 +- src/backend/commands/repack.c | 2 +- src/backend/commands/vacuum.c | 2 +- src/backend/postmaster/checkpointer.c | 2 +- src/include/commands/defrem.h | 1 + 6 files changed, 60 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/define.c b/src/backend/commands/define.c index 4172cc9bacb..81d009e9866 100644 --- a/src/backend/commands/define.c +++ b/src/backend/commands/define.c @@ -374,3 +374,58 @@ errorConflictingDefElem(DefElem *defel, ParseState *pstate) errmsg("conflicting or redundant options"), parser_errposition(pstate, defel->location)); } + +/* + * Collapse a DefElem option list to "last specification wins" semantics. + * + * Several utility commands (VACUUM, EXPLAIN, CHECKPOINT, REPACK, ...) accept a + * parenthesized list of options and intend that, when an option is given more + * than once, the last specification takes effect. Historically each command + * implemented its own parse loop, and getting last-wins right for every option + * was easy to botch -- for example a loop that OR'd a boolean would keep an + * option enabled even if its final specification was OFF. + * + * This helper centralizes that behavior. It returns a newly-built list that + * contains, for each distinct option name (compared case-sensitively on + * defname), only the DefElem for its last occurrence in the input, keeping the + * relative order of those surviving entries. Callers can then run their + * existing option-parsing loop over the result without worrying about repeated + * options at all. + * + * The input list is not modified; the DefElem nodes themselves are not copied, + * only referenced from the returned list. A NIL input yields NIL. + */ +List * +deduplicateDefElemList(List *options) +{ + List *result = NIL; + ListCell *outer; + + foreach(outer, options) + { + DefElem *opt = (DefElem *) lfirst(outer); + bool superseded = false; + ListCell *inner; + + /* + * Keep this entry only if no later entry carries the same option + * name. This is O(n^2) in the number of options, but option lists are + * short (a handful of entries), so a hash table would be overkill. + */ + for_each_cell(inner, options, lnext(options, outer)) + { + DefElem *later = (DefElem *) lfirst(inner); + + if (strcmp(opt->defname, later->defname) == 0) + { + superseded = true; + break; + } + } + + if (!superseded) + result = lappend(result, opt); + } + + return result; +} diff --git a/src/backend/commands/explain_state.c b/src/backend/commands/explain_state.c index 9ddfdaea54d..95f8b986353 100644 --- a/src/backend/commands/explain_state.c +++ b/src/backend/commands/explain_state.c @@ -85,7 +85,7 @@ ParseExplainOptionList(ExplainState *es, List *options, ParseState *pstate) bool summary_set = false; /* Parse options list. */ - foreach(lc, options) + foreach(lc, deduplicateDefElemList(options)) { DefElem *opt = (DefElem *) lfirst(lc); diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index a584e138ada..14463eb3f66 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -269,7 +269,7 @@ ExecRepack(ParseState *pstate, RepackStmt *stmt, bool isTopLevel) bool concurrently = false; /* Parse option list */ - foreach_node(DefElem, opt, stmt->params) + foreach_node(DefElem, opt, deduplicateDefElemList(stmt->params)) { if (strcmp(opt->defname, "verbose") == 0) verbose = defGetBoolean(opt); diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index a257dd8d21e..7d4c68f3b66 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -197,7 +197,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) ring_size = -1; /* Parse options list */ - foreach(lc, vacstmt->options) + foreach(lc, deduplicateDefElemList(vacstmt->options)) { DefElem *opt = (DefElem *) lfirst(lc); diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index 580c7944119..7f6f6590cd7 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -1003,7 +1003,7 @@ ExecCheckpoint(ParseState *pstate, CheckPointStmt *stmt) bool fast = true; bool unlogged = false; - foreach_ptr(DefElem, opt, stmt->options) + foreach_ptr(DefElem, opt, deduplicateDefElemList(stmt->options)) { if (strcmp(opt->defname, "mode") == 0) { diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h index 574f860bdd2..92a1473eb82 100644 --- a/src/include/commands/defrem.h +++ b/src/include/commands/defrem.h @@ -162,5 +162,6 @@ extern TypeName *defGetTypeName(DefElem *def); extern int defGetTypeLength(DefElem *def); extern List *defGetStringList(DefElem *def); pg_noreturn extern void errorConflictingDefElem(DefElem *defel, ParseState *pstate); +extern List *deduplicateDefElemList(List *options); #endif /* DEFREM_H */ -- 2.54.0 (Apple Git-157)