From 6776142a07afb4c28961f27059d800196902f5f1 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Mon, 18 Mar 2019 15:14:52 -0400 Subject: [PATCH] Revise parse tree representation for VACUUM and ANALYZE. Like commit f41551f61f9cf4eedd5b7173f985a3bdb4d9858c, this aims to make it easier to add non-Boolean options to VACUUM (or, in this case, to ANALYZE). Instead of building up a bitmap of options directly in the parser, build up a list of DefElem objects and let ExecVacuum() sort it out; right now, we make no use of the fact that a DefElem can carry an associated value, but it will be easy to make that change in the future. Masahiko Sawada Discussion: http://postgr.es/m/CAD21AoATE4sn0jFFH3NcfUZXkU2BMbjBWB_kDj-XWYA-LXDcQA@mail.gmail.com --- src/backend/commands/vacuum.c | 52 ++++++++++++--- src/backend/parser/gram.y | 94 +++++++++++++--------------- src/backend/tcop/utility.c | 6 +- src/include/commands/vacuum.h | 17 ++++- src/include/nodes/parsenodes.h | 26 +++----- src/test/regress/expected/vacuum.out | 6 +- src/test/regress/sql/vacuum.sql | 1 + 7 files changed, 116 insertions(+), 86 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index be03185a7f..f0afeaf40c 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -83,20 +83,55 @@ static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params); * happen in vacuum(). */ void -ExecVacuum(VacuumStmt *vacstmt, bool isTopLevel) +ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) { VacuumParams params; + ListCell *lc; + + params.options = vacstmt->is_vacuumcmd ? VACOPT_VACUUM : VACOPT_ANALYZE; + + /* Parse options list */ + foreach(lc, vacstmt->options) + { + DefElem *opt = (DefElem *) lfirst(lc); + + /* Parse common options for VACUUM and ANALYZE */ + if (strcmp(opt->defname, "verbose") == 0) + params.options |= VACOPT_VERBOSE; + else if (strcmp(opt->defname, "skip_locked") == 0) + params.options |= VACOPT_SKIP_LOCKED; + else if (!vacstmt->is_vacuumcmd) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("unrecognized ANALYZE option \"%s\"", opt->defname), + parser_errposition(pstate, opt->location))); + + /* Parse options available on VACUUM */ + else if (strcmp(opt->defname, "analyze") == 0) + params.options |= VACOPT_ANALYZE; + else if (strcmp(opt->defname, "freeze") == 0) + params.options |= VACOPT_FREEZE; + else if (strcmp(opt->defname, "full") == 0) + params.options |= VACOPT_FULL; + else if (strcmp(opt->defname, "disable_page_skipping") == 0) + params.options |= VACOPT_DISABLE_PAGE_SKIPPING; + else + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("unrecognized VACUUM option \"%s\"", opt->defname), + parser_errposition(pstate, opt->location))); + } /* sanity checks on options */ - Assert(vacstmt->options & (VACOPT_VACUUM | VACOPT_ANALYZE)); - Assert((vacstmt->options & VACOPT_VACUUM) || - !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE))); - Assert(!(vacstmt->options & VACOPT_SKIPTOAST)); + Assert(params.options & (VACOPT_VACUUM | VACOPT_ANALYZE)); + Assert((params.options & VACOPT_VACUUM) || + !(params.options & (VACOPT_FULL | VACOPT_FREEZE))); + Assert(!(params.options & VACOPT_SKIPTOAST)); /* * Make sure VACOPT_ANALYZE is specified if any column lists are present. */ - if (!(vacstmt->options & VACOPT_ANALYZE)) + if (!(params.options & VACOPT_ANALYZE)) { ListCell *lc; @@ -111,14 +146,11 @@ ExecVacuum(VacuumStmt *vacstmt, bool isTopLevel) } } - /* copy options from parse tree */ - params.options = vacstmt->options; - /* * All freeze ages are zero if the FREEZE option is given; otherwise pass * them as -1 which means to use the default values. */ - if (vacstmt->options & VACOPT_FREEZE) + if (params.options & VACOPT_FREEZE) { params.freeze_min_age = 0; params.freeze_table_age = 0; diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index e23e68fdb3..e814939a25 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -306,8 +306,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); create_extension_opt_item alter_extension_opt_item %type opt_lock lock_type cast_context -%type vacuum_option_list vacuum_option_elem - analyze_option_list analyze_option_elem +%type vac_analyze_option_name +%type vac_analyze_option_elem +%type vac_analyze_option_list %type opt_or_replace opt_grant_grant_option opt_grant_admin_option opt_nowait opt_if_exists opt_with_data @@ -10460,85 +10461,62 @@ cluster_index_specification: VacuumStmt: VACUUM opt_full opt_freeze opt_verbose opt_analyze opt_vacuum_relation_list { VacuumStmt *n = makeNode(VacuumStmt); - n->options = VACOPT_VACUUM; + n->options = NIL; if ($2) - n->options |= VACOPT_FULL; + n->options = lappend(n->options, + makeDefElem("full", NULL, @2)); if ($3) - n->options |= VACOPT_FREEZE; + n->options = lappend(n->options, + makeDefElem("freeze", NULL, @3)); if ($4) - n->options |= VACOPT_VERBOSE; + n->options = lappend(n->options, + makeDefElem("verbose", NULL, @4)); if ($5) - n->options |= VACOPT_ANALYZE; + n->options = lappend(n->options, + makeDefElem("analyze", NULL, @5)); n->rels = $6; + n->is_vacuumcmd = true; $$ = (Node *)n; } - | VACUUM '(' vacuum_option_list ')' opt_vacuum_relation_list + | VACUUM '(' vac_analyze_option_list ')' opt_vacuum_relation_list { VacuumStmt *n = makeNode(VacuumStmt); - n->options = VACOPT_VACUUM | $3; + n->options = $3; n->rels = $5; + n->is_vacuumcmd = true; $$ = (Node *) n; } ; -vacuum_option_list: - vacuum_option_elem { $$ = $1; } - | vacuum_option_list ',' vacuum_option_elem { $$ = $1 | $3; } - ; - -vacuum_option_elem: - analyze_keyword { $$ = VACOPT_ANALYZE; } - | VERBOSE { $$ = VACOPT_VERBOSE; } - | FREEZE { $$ = VACOPT_FREEZE; } - | FULL { $$ = VACOPT_FULL; } - | IDENT - { - if (strcmp($1, "disable_page_skipping") == 0) - $$ = VACOPT_DISABLE_PAGE_SKIPPING; - else if (strcmp($1, "skip_locked") == 0) - $$ = VACOPT_SKIP_LOCKED; - else - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("unrecognized VACUUM option \"%s\"", $1), - parser_errposition(@1))); - } - ; - AnalyzeStmt: analyze_keyword opt_verbose opt_vacuum_relation_list { VacuumStmt *n = makeNode(VacuumStmt); - n->options = VACOPT_ANALYZE; + n->options = NIL; if ($2) - n->options |= VACOPT_VERBOSE; + n->options = lappend(n->options, + makeDefElem("verbose", NULL, @2)); n->rels = $3; + n->is_vacuumcmd = false; $$ = (Node *)n; } - | analyze_keyword '(' analyze_option_list ')' opt_vacuum_relation_list + | analyze_keyword '(' vac_analyze_option_list ')' opt_vacuum_relation_list { VacuumStmt *n = makeNode(VacuumStmt); - n->options = VACOPT_ANALYZE | $3; + n->options = $3; n->rels = $5; + n->is_vacuumcmd = false; $$ = (Node *) n; } ; -analyze_option_list: - analyze_option_elem { $$ = $1; } - | analyze_option_list ',' analyze_option_elem { $$ = $1 | $3; } - ; - -analyze_option_elem: - VERBOSE { $$ = VACOPT_VERBOSE; } - | IDENT +vac_analyze_option_list: + vac_analyze_option_elem { - if (strcmp($1, "skip_locked") == 0) - $$ = VACOPT_SKIP_LOCKED; - else - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("unrecognized ANALYZE option \"%s\"", $1), - parser_errposition(@1))); + $$ = list_make1($1); + } + | vac_analyze_option_list ',' vac_analyze_option_elem + { + $$ = lappend($1, $3); } ; @@ -10547,6 +10525,18 @@ analyze_keyword: | ANALYSE /* British */ {} ; +vac_analyze_option_elem: + vac_analyze_option_name + { + $$ = makeDefElem($1, NULL, @1); + } + ; + +vac_analyze_option_name: + NonReservedWord { $$ = $1; } + | analyze_keyword { $$ = "analyze"; } + ; + opt_analyze: analyze_keyword { $$ = true; } | /*EMPTY*/ { $$ = false; } diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 6ec795f1b4..bdfaa506e7 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -664,10 +664,10 @@ standard_ProcessUtility(PlannedStmt *pstmt, VacuumStmt *stmt = (VacuumStmt *) parsetree; /* we choose to allow this during "read only" transactions */ - PreventCommandDuringRecovery((stmt->options & VACOPT_VACUUM) ? + PreventCommandDuringRecovery(stmt->is_vacuumcmd ? "VACUUM" : "ANALYZE"); /* forbidden in parallel mode due to CommandIsReadOnly */ - ExecVacuum(stmt, isTopLevel); + ExecVacuum(pstate, stmt, isTopLevel); } break; @@ -2570,7 +2570,7 @@ CreateCommandTag(Node *parsetree) break; case T_VacuumStmt: - if (((VacuumStmt *) parsetree)->options & VACOPT_VACUUM) + if (((VacuumStmt *) parsetree)->is_vacuumcmd) tag = "VACUUM"; else tag = "ANALYZE"; diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index c0df9c9054..77086f3e91 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -136,8 +136,23 @@ typedef struct VacAttrStats int rowstride; } VacAttrStats; +typedef enum VacuumOption +{ + VACOPT_VACUUM = 1 << 0, /* do VACUUM */ + VACOPT_ANALYZE = 1 << 1, /* do ANALYZE */ + VACOPT_VERBOSE = 1 << 2, /* print progress info */ + VACOPT_FREEZE = 1 << 3, /* FREEZE option */ + VACOPT_FULL = 1 << 4, /* FULL (non-concurrent) vacuum */ + VACOPT_SKIP_LOCKED = 1 << 5, /* skip if cannot get lock */ + VACOPT_SKIPTOAST = 1 << 6, /* don't process the TOAST table, if any */ + VACOPT_DISABLE_PAGE_SKIPPING = 1 << 7 /* don't skip any pages */ +} VacuumOption; + /* * Parameters customizing behavior of VACUUM and ANALYZE. + * + * Note that at least one of VACOPT_VACUUM and VACOPT_ANALYZE must be set + * in options. */ typedef struct VacuumParams { @@ -163,7 +178,7 @@ extern int vacuum_multixact_freeze_table_age; /* in commands/vacuum.c */ -extern void ExecVacuum(VacuumStmt *vacstmt, bool isTopLevel); +extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel); extern void vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy, bool isTopLevel); extern void vac_open_indexes(Relation relation, LOCKMODE lockmode, diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index fe35783359..fcfba4be4c 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -3151,21 +3151,16 @@ typedef struct ClusterStmt * Vacuum and Analyze Statements * * Even though these are nominally two statements, it's convenient to use - * just one node type for both. Note that at least one of VACOPT_VACUUM - * and VACOPT_ANALYZE must be set in options. + * just one node type for both. * ---------------------- */ -typedef enum VacuumOption +typedef struct VacuumStmt { - VACOPT_VACUUM = 1 << 0, /* do VACUUM */ - VACOPT_ANALYZE = 1 << 1, /* do ANALYZE */ - VACOPT_VERBOSE = 1 << 2, /* print progress info */ - VACOPT_FREEZE = 1 << 3, /* FREEZE option */ - VACOPT_FULL = 1 << 4, /* FULL (non-concurrent) vacuum */ - VACOPT_SKIP_LOCKED = 1 << 5, /* skip if cannot get lock */ - VACOPT_SKIPTOAST = 1 << 6, /* don't process the TOAST table, if any */ - VACOPT_DISABLE_PAGE_SKIPPING = 1 << 7 /* don't skip any pages */ -} VacuumOption; + NodeTag type; + List *options; /* list of DefElem nodes */ + List *rels; /* list of VacuumRelation, or NIL for all */ + bool is_vacuumcmd; /* true for VACUUM, false for ANALYZE */ +} VacuumStmt; /* * Info about a single target table of VACUUM/ANALYZE. @@ -3182,13 +3177,6 @@ typedef struct VacuumRelation List *va_cols; /* list of column names, or NIL for all */ } VacuumRelation; -typedef struct VacuumStmt -{ - NodeTag type; - int options; /* OR of VacuumOption flags */ - List *rels; /* list of VacuumRelation, or NIL for all */ -} VacuumStmt; - /* ---------------------- * Explain Statement * diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index fa9d663abd..07d0703115 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -116,8 +116,12 @@ ERROR: column "does_not_exist" of relation "vacparted" does not exist ANALYZE (VERBOSE) does_not_exist; ERROR: relation "does_not_exist" does not exist ANALYZE (nonexistent-arg) does_not_exist; -ERROR: unrecognized ANALYZE option "nonexistent" +ERROR: syntax error at or near "-" LINE 1: ANALYZE (nonexistent-arg) does_not_exist; + ^ +ANALYZE (nonexistentarg) does_not_exit; +ERROR: unrecognized ANALYZE option "nonexistentarg" +LINE 1: ANALYZE (nonexistentarg) does_not_exit; ^ -- ensure argument order independence, and that SKIP_LOCKED on non-existing -- relation still errors out. diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql index 9defa0d8b2..81f3822679 100644 --- a/src/test/regress/sql/vacuum.sql +++ b/src/test/regress/sql/vacuum.sql @@ -92,6 +92,7 @@ ANALYZE vactst (i), vacparted (does_not_exist); -- parenthesized syntax for ANALYZE ANALYZE (VERBOSE) does_not_exist; ANALYZE (nonexistent-arg) does_not_exist; +ANALYZE (nonexistentarg) does_not_exit; -- ensure argument order independence, and that SKIP_LOCKED on non-existing -- relation still errors out.