From fed84348477785f8b0b2ae3a1caf2e927e405f96 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Fri, 27 Mar 2020 17:50:46 -0500 Subject: [PATCH v22 1/5] Change REINDEX/CLUSTER to accept an option list.. ..like EXPLAIN (..), VACUUM (..), and ANALYZE (..). Change docs in the style of VACUUM. See also: 52dcfda48778d16683c64ca4372299a099a15b96 --- doc/src/sgml/ref/cluster.sgml | 29 ++++++++++++++++++--- doc/src/sgml/ref/reindex.sgml | 43 +++++++++++++++++++++----------- src/backend/commands/cluster.c | 23 ++++++++++++++++- src/backend/commands/indexcmds.c | 41 ++++++++++++++++-------------- src/backend/nodes/copyfuncs.c | 2 ++ src/backend/nodes/equalfuncs.c | 2 ++ src/backend/parser/gram.y | 35 +++++++++++++++----------- src/backend/tcop/utility.c | 36 +++++++++++++++++++++++--- src/bin/psql/tab-complete.c | 23 +++++++++++++---- src/include/commands/cluster.h | 3 ++- src/include/commands/defrem.h | 7 +++--- src/include/nodes/parsenodes.h | 2 ++ 12 files changed, 180 insertions(+), 66 deletions(-) diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 4da60d8d56..a6df8a3d81 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -21,8 +21,13 @@ PostgreSQL documentation -CLUSTER [VERBOSE] table_name [ USING index_name ] -CLUSTER [VERBOSE] +CLUSTER [VERBOSE] [ ( option [, ...] ) ] table_name [ USING index_name ] +CLUSTER [VERBOSE] [ ( option [, ...] ) ] + +where option can be one of: + + VERBOSE [ boolean ] + @@ -81,6 +86,16 @@ CLUSTER [VERBOSE] Parameters + + VERBOSE + + + Prints a progress report as each table is clustered. + + + + + table_name @@ -100,13 +115,19 @@ CLUSTER [VERBOSE] - VERBOSE + boolean - Prints a progress report as each table is clustered. + Specifies whether the selected option should be turned on or off. + You can write TRUE, ON, or + 1 to enable the option, and FALSE, + OFF, or 0 to disable it. The + boolean value can also + be omitted, in which case TRUE is assumed. + diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index aac5d5be23..ff369a7313 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -25,7 +25,7 @@ REINDEX [ ( option [, ...] ) ] { IN where option can be one of: - VERBOSE + VERBOSE [ boolean ] @@ -141,19 +141,6 @@ REINDEX [ ( option [, ...] ) ] { IN - - name - - - The name of the specific index, table, or database to be - reindexed. Index and table names can be schema-qualified. - Presently, REINDEX DATABASE and REINDEX SYSTEM - can only reindex the current database, so their parameter must match - the current database's name. - - - - CONCURRENTLY @@ -181,6 +168,34 @@ REINDEX [ ( option [, ...] ) ] { IN + + + name + + + The name of the specific index, table, or database to be + reindexed. Index and table names can be schema-qualified. + Presently, REINDEX DATABASE and REINDEX SYSTEM + can only reindex the current database, so their parameter must match + the current database's name. + + + + + + boolean + + + Specifies whether the selected option should be turned on or off. + You can write TRUE, ON, or + 1 to enable the option, and FALSE, + OFF, or 0 to disable it. The + boolean value can also + be omitted, in which case TRUE is assumed. + + + + diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 04d12a7ece..1817b8829a 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -35,6 +35,7 @@ #include "catalog/pg_am.h" #include "catalog/toasting.h" #include "commands/cluster.h" +#include "commands/defrem.h" #include "commands/progress.h" #include "commands/tablecmds.h" #include "commands/vacuum.h" @@ -99,8 +100,28 @@ static List *get_tables_to_cluster(MemoryContext cluster_context); *--------------------------------------------------------------------------- */ void -cluster(ClusterStmt *stmt, bool isTopLevel) +cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) { + ListCell *lc; + + /* Parse list of generic parameters not handled by the parser */ + foreach(lc, stmt->params) + { + DefElem *opt = (DefElem *) lfirst(lc); + + if (strcmp(opt->defname, "verbose") == 0) + if (defGetBoolean(opt)) + stmt->options |= CLUOPT_VERBOSE; + else + stmt->options &= ~CLUOPT_VERBOSE; + else + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("unrecognized CLUSTER option \"%s\"", + opt->defname), + parser_errposition(pstate, opt->location))); + } + if (stmt->relation != NULL) { /* This is the single-relation case. */ diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 7819266a63..a22b32fc74 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2420,8 +2420,9 @@ ChooseIndexColumnNames(List *indexElems) * Recreate a specific index. */ void -ReindexIndex(RangeVar *indexRelation, int options, bool concurrent) +ReindexIndex(ReindexStmt *stmt) { + RangeVar *indexRelation = stmt->relation; struct ReindexIndexCallbackState state; Oid indOid; Relation irel; @@ -2437,10 +2438,10 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent) * upgrade the lock, but that's OK, because other sessions can't hold * locks on our temporary table. */ - state.concurrent = concurrent; + state.concurrent = stmt->concurrent; state.locked_table_oid = InvalidOid; indOid = RangeVarGetRelidExtended(indexRelation, - concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock, + stmt->concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock, 0, RangeVarCallbackForReindexIndex, &state); @@ -2460,11 +2461,11 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent) persistence = irel->rd_rel->relpersistence; index_close(irel, NoLock); - if (concurrent && persistence != RELPERSISTENCE_TEMP) - ReindexRelationConcurrently(indOid, options); + if (stmt->concurrent && persistence != RELPERSISTENCE_TEMP) + ReindexRelationConcurrently(indOid, stmt->options); else reindex_index(indOid, false, persistence, - options | REINDEXOPT_REPORT_PROGRESS); + stmt->options | REINDEXOPT_REPORT_PROGRESS); } /* @@ -2542,8 +2543,9 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation, * Recreate all indexes of a table (and of its toast table, if any) */ Oid -ReindexTable(RangeVar *relation, int options, bool concurrent) +ReindexTable(ReindexStmt *stmt) { + RangeVar *relation = stmt->relation; Oid heapOid; bool result; @@ -2556,13 +2558,13 @@ ReindexTable(RangeVar *relation, int options, bool concurrent) * locks on our temporary table. */ heapOid = RangeVarGetRelidExtended(relation, - concurrent ? ShareUpdateExclusiveLock : ShareLock, + stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock, 0, RangeVarCallbackOwnsTable, NULL); - if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP) + if (stmt->concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP) { - result = ReindexRelationConcurrently(heapOid, options); + result = ReindexRelationConcurrently(heapOid, stmt->options); if (!result) ereport(NOTICE, @@ -2574,7 +2576,7 @@ ReindexTable(RangeVar *relation, int options, bool concurrent) result = reindex_relation(heapOid, REINDEX_REL_PROCESS_TOAST | REINDEX_REL_CHECK_CONSTRAINTS, - options | REINDEXOPT_REPORT_PROGRESS); + stmt->options | REINDEXOPT_REPORT_PROGRESS); if (!result) ereport(NOTICE, (errmsg("table \"%s\" has no indexes to reindex", @@ -2593,9 +2595,10 @@ ReindexTable(RangeVar *relation, int options, bool concurrent) * That means this must not be called within a user transaction block! */ void -ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, - int options, bool concurrent) +ReindexMultipleTables(ReindexStmt *stmt) { + const char *objectName = stmt->name; + ReindexObjectType objectKind = stmt->kind; Oid objectOid; Relation relationRelation; TableScanDesc scan; @@ -2613,7 +2616,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, objectKind == REINDEX_OBJECT_SYSTEM || objectKind == REINDEX_OBJECT_DATABASE); - if (objectKind == REINDEX_OBJECT_SYSTEM && concurrent) + if (objectKind == REINDEX_OBJECT_SYSTEM && stmt->concurrent) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot reindex system catalogs concurrently"))); @@ -2724,7 +2727,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, * Skip system tables, since index_create() would reject indexing them * concurrently (and it would likely fail if we tried). */ - if (concurrent && + if (stmt->concurrent && IsCatalogRelationOid(relid)) { if (!concurrent_warning) @@ -2766,9 +2769,9 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, /* functions in indexes may want a snapshot set */ PushActiveSnapshot(GetTransactionSnapshot()); - if (concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP) + if (stmt->concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP) { - (void) ReindexRelationConcurrently(relid, options); + (void) ReindexRelationConcurrently(relid, stmt->options); /* ReindexRelationConcurrently() does the verbose output */ } else @@ -2778,9 +2781,9 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, result = reindex_relation(relid, REINDEX_REL_PROCESS_TOAST | REINDEX_REL_CHECK_CONSTRAINTS, - options | REINDEXOPT_REPORT_PROGRESS); + stmt->options | REINDEXOPT_REPORT_PROGRESS); - if (result && (options & REINDEXOPT_VERBOSE)) + if (result && (stmt->options & REINDEXOPT_VERBOSE)) ereport(INFO, (errmsg("table \"%s.%s\" was reindexed", get_namespace_name(get_rel_namespace(relid)), diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 89c409de66..71548acc0c 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -3358,6 +3358,7 @@ _copyClusterStmt(const ClusterStmt *from) COPY_NODE_FIELD(relation); COPY_STRING_FIELD(indexname); COPY_SCALAR_FIELD(options); + COPY_NODE_FIELD(params); return newnode; } @@ -4450,6 +4451,7 @@ _copyReindexStmt(const ReindexStmt *from) COPY_NODE_FIELD(relation); COPY_STRING_FIELD(name); COPY_SCALAR_FIELD(options); + COPY_NODE_FIELD(params); COPY_SCALAR_FIELD(concurrent); return newnode; diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index e3f33c40be..de48a42cdd 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -1217,6 +1217,7 @@ _equalClusterStmt(const ClusterStmt *a, const ClusterStmt *b) COMPARE_NODE_FIELD(relation); COMPARE_STRING_FIELD(indexname); COMPARE_SCALAR_FIELD(options); + COMPARE_NODE_FIELD(params); return true; } @@ -2135,6 +2136,7 @@ _equalReindexStmt(const ReindexStmt *a, const ReindexStmt *b) COMPARE_NODE_FIELD(relation); COMPARE_STRING_FIELD(name); COMPARE_SCALAR_FIELD(options); + COMPARE_NODE_FIELD(params); COMPARE_SCALAR_FIELD(concurrent); return true; diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index dbb47d4982..b909b161a6 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -519,7 +519,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type explain_option_list %type reindex_target_type reindex_target_multitable -%type reindex_option_list reindex_option_elem %type copy_generic_opt_arg copy_generic_opt_arg_list_item %type copy_generic_opt_elem @@ -8180,7 +8179,7 @@ ReindexStmt: n->concurrent = $3; n->relation = $4; n->name = NULL; - n->options = 0; + n->params = NIL; $$ = (Node *)n; } | REINDEX reindex_target_multitable opt_concurrently name @@ -8190,27 +8189,27 @@ ReindexStmt: n->concurrent = $3; n->name = $4; n->relation = NULL; - n->options = 0; + n->params = NIL; $$ = (Node *)n; } - | REINDEX '(' reindex_option_list ')' reindex_target_type opt_concurrently qualified_name + | REINDEX '(' vac_analyze_option_list ')' reindex_target_type opt_concurrently qualified_name { ReindexStmt *n = makeNode(ReindexStmt); n->kind = $5; n->concurrent = $6; n->relation = $7; n->name = NULL; - n->options = $3; + n->params = $3; $$ = (Node *)n; } - | REINDEX '(' reindex_option_list ')' reindex_target_multitable opt_concurrently name + | REINDEX '(' vac_analyze_option_list ')' reindex_target_multitable opt_concurrently name { ReindexStmt *n = makeNode(ReindexStmt); n->kind = $5; n->concurrent = $6; n->name = $7; n->relation = NULL; - n->options = $3; + n->params = $3; $$ = (Node *)n; } ; @@ -8223,13 +8222,6 @@ reindex_target_multitable: | SYSTEM_P { $$ = REINDEX_OBJECT_SYSTEM; } | DATABASE { $$ = REINDEX_OBJECT_DATABASE; } ; -reindex_option_list: - reindex_option_elem { $$ = $1; } - | reindex_option_list ',' reindex_option_elem { $$ = $1 | $3; } - ; -reindex_option_elem: - VERBOSE { $$ = REINDEXOPT_VERBOSE; } - ; /***************************************************************************** * @@ -10380,6 +10372,7 @@ CreateConversionStmt: * * QUERY: * CLUSTER [VERBOSE] [ USING ] + * CLUSTER [VERBOSE] [(options)] [ USING ] * CLUSTER [VERBOSE] * CLUSTER [VERBOSE] ON (for pre-8.3) * @@ -10394,6 +10387,18 @@ ClusterStmt: n->options = 0; if ($2) n->options |= CLUOPT_VERBOSE; + n->params = NIL; + $$ = (Node*)n; + } + + | CLUSTER opt_verbose '(' vac_analyze_option_list ')' qualified_name cluster_index_specification + { + ClusterStmt *n = makeNode(ClusterStmt); + n->relation = $6; + n->indexname = $7; + if ($2) + n->options |= CLUOPT_VERBOSE; + n->params = $4; $$ = (Node*)n; } | CLUSTER opt_verbose @@ -10404,6 +10409,7 @@ ClusterStmt: n->options = 0; if ($2) n->options |= CLUOPT_VERBOSE; + n->params = NIL; $$ = (Node*)n; } /* kept for pre-8.3 compatibility */ @@ -10415,6 +10421,7 @@ ClusterStmt: n->options = 0; if ($2) n->options |= CLUOPT_VERBOSE; + n->params = NIL; $$ = (Node*)n; } ; diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 9b0c376c8c..c48983a5de 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -526,6 +526,33 @@ ProcessUtility(PlannedStmt *pstmt, dest, qc); } +/* Parse params not parsed by the grammar */ +static +void parse_reindex_params(ParseState *pstate, ReindexStmt *stmt) +{ + ListCell *lc; + foreach(lc, stmt->params) + { + DefElem *opt = (DefElem *) lfirst(lc); + + if (strcmp(opt->defname, "verbose") == 0) + { + if (defGetBoolean(opt)) + stmt->options |= REINDEXOPT_VERBOSE; + else + stmt->options &= ~REINDEXOPT_VERBOSE; + } + else if (strcmp(opt->defname, "concurrently") == 0) + stmt->concurrent = true; + else + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("unrecognized REINDEX option \"%s\"", + opt->defname), + parser_errposition(pstate, opt->location))); + } +} + /* * standard_ProcessUtility itself deals only with utility commands for * which we do not provide event trigger support. Commands that do have @@ -818,7 +845,7 @@ standard_ProcessUtility(PlannedStmt *pstmt, break; case T_ClusterStmt: - cluster((ClusterStmt *) parsetree, isTopLevel); + cluster(pstate, (ClusterStmt *) parsetree, isTopLevel); break; case T_VacuumStmt: @@ -923,13 +950,14 @@ standard_ProcessUtility(PlannedStmt *pstmt, PreventInTransactionBlock(isTopLevel, "REINDEX CONCURRENTLY"); + parse_reindex_params(pstate, stmt); switch (stmt->kind) { case REINDEX_OBJECT_INDEX: - ReindexIndex(stmt->relation, stmt->options, stmt->concurrent); + ReindexIndex(stmt); break; case REINDEX_OBJECT_TABLE: - ReindexTable(stmt->relation, stmt->options, stmt->concurrent); + ReindexTable(stmt); break; case REINDEX_OBJECT_SCHEMA: case REINDEX_OBJECT_SYSTEM: @@ -945,7 +973,7 @@ standard_ProcessUtility(PlannedStmt *pstmt, (stmt->kind == REINDEX_OBJECT_SCHEMA) ? "REINDEX SCHEMA" : (stmt->kind == REINDEX_OBJECT_SYSTEM) ? "REINDEX SYSTEM" : "REINDEX DATABASE"); - ReindexMultipleTables(stmt->name, stmt->kind, stmt->options, stmt->concurrent); + ReindexMultipleTables(stmt); break; default: elog(ERROR, "unrecognized object type: %d", diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index f41785f11c..de85772ff9 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -2259,21 +2259,34 @@ psql_completion(const char *text, int start, int end) /* CLUSTER */ else if (Matches("CLUSTER")) COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_clusterables, "UNION SELECT 'VERBOSE'"); - else if (Matches("CLUSTER", "VERBOSE")) + else if (Matches("CLUSTER", "VERBOSE") || + Matches("CLUSTER", "VERBOSE", "(*)") || + Matches("CLUSTER", "(*)")) COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_clusterables, NULL); /* If we have CLUSTER , then add "USING" */ - else if (Matches("CLUSTER", MatchAnyExcept("VERBOSE|ON"))) + else if (Matches("CLUSTER", MatchAnyExcept("VERBOSE|ON|(|(*)"))) COMPLETE_WITH("USING"); /* If we have CLUSTER VERBOSE , then add "USING" */ - else if (Matches("CLUSTER", "VERBOSE", MatchAny)) + else if (Matches("CLUSTER", "VERBOSE|(*)", MatchAny)) COMPLETE_WITH("USING"); /* If we have CLUSTER USING, then add the index as well */ else if (Matches("CLUSTER", MatchAny, "USING") || - Matches("CLUSTER", "VERBOSE", MatchAny, "USING")) + Matches("CLUSTER", "VERBOSE|(*)", MatchAny, "USING")) { completion_info_charp = prev2_wd; COMPLETE_WITH_QUERY(Query_for_index_of_table); } + else if (HeadMatches("CLUSTER", "(*") && + !HeadMatches("CLUSTER", "(*)")) + { + /* + * This fires if we're in an unfinished parenthesized option list. + * get_previous_words treats a completed parenthesized option list as + * one word, so the above test is correct. + */ + if (ends_with(prev_wd, '(') || ends_with(prev_wd, ',')) + COMPLETE_WITH("VERBOSE"); + } /* COMMENT */ else if (Matches("COMMENT")) @@ -3470,7 +3483,7 @@ psql_completion(const char *text, int start, int end) * one word, so the above test is correct. */ if (ends_with(prev_wd, '(') || ends_with(prev_wd, ',')) - COMPLETE_WITH("VERBOSE"); + COMPLETE_WITH("CONCURRENTLY", "VERBOSE"); } /* SECURITY LABEL */ diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h index e05884781b..674cdcd0cd 100644 --- a/src/include/commands/cluster.h +++ b/src/include/commands/cluster.h @@ -14,11 +14,12 @@ #define CLUSTER_H #include "nodes/parsenodes.h" +#include "parser/parse_node.h" #include "storage/lock.h" #include "utils/relcache.h" -extern void cluster(ClusterStmt *stmt, bool isTopLevel); +extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel); extern void cluster_rel(Oid tableOid, Oid indexOid, int options); extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMODE lockmode); diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h index c26a102b17..a97c00a02e 100644 --- a/src/include/commands/defrem.h +++ b/src/include/commands/defrem.h @@ -34,10 +34,9 @@ extern ObjectAddress DefineIndex(Oid relationId, bool check_not_in_use, bool skip_build, bool quiet); -extern void ReindexIndex(RangeVar *indexRelation, int options, bool concurrent); -extern Oid ReindexTable(RangeVar *relation, int options, bool concurrent); -extern void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, - int options, bool concurrent); +extern void ReindexIndex(ReindexStmt *stmt); +extern Oid ReindexTable(ReindexStmt *stmt); +extern void ReindexMultipleTables(ReindexStmt *stmt); extern char *makeObjectName(const char *name1, const char *name2, const char *label); extern char *ChooseRelationName(const char *name1, const char *name2, diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 151bcdb7ef..295c2802a4 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -3210,6 +3210,7 @@ typedef struct ClusterStmt RangeVar *relation; /* relation being indexed, or NULL if all */ char *indexname; /* original index defined */ int options; /* OR of ClusterOption flags */ + List *params; /* Params not further parsed by the grammar */ } ClusterStmt; /* ---------------------- @@ -3369,6 +3370,7 @@ typedef struct ReindexStmt RangeVar *relation; /* Table or index to reindex */ const char *name; /* name of database to reindex */ int options; /* Reindex options flags */ + List *params; /* Params not further parsed by the grammer */ bool concurrent; /* reindex concurrently? */ } ReindexStmt; -- 2.17.0