From 62365ff3f8a1b75f8e529aa4603a92cbac950257 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Wed, 2 Sep 2020 17:20:37 -0500 Subject: [PATCH v25 2/6] Deprecate ReindexStmt->concurrent and options ..since parenthesized syntax is the modern syntax, remove the special field for handling nonparenthesized, boolean-only option. Should be squished with previous commit. --- src/backend/commands/cluster.c | 11 +++++---- src/backend/commands/indexcmds.c | 39 +++++++++++++++++--------------- src/backend/nodes/copyfuncs.c | 3 --- src/backend/nodes/equalfuncs.c | 3 --- src/backend/parser/gram.y | 31 ++++++++++++++----------- src/backend/tcop/utility.c | 22 ++++++++++-------- src/include/commands/defrem.h | 6 ++--- src/include/nodes/parsenodes.h | 8 +++---- 8 files changed, 64 insertions(+), 59 deletions(-) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index a42dfe98f4..4a3678831d 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -105,7 +105,8 @@ static List *get_tables_to_cluster(MemoryContext cluster_context); void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) { - ListCell *lc; + ListCell *lc; + int options = 0; /* Parse list of generic parameters not handled by the parser */ foreach(lc, stmt->params) @@ -114,9 +115,9 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) if (strcmp(opt->defname, "verbose") == 0) if (defGetBoolean(opt)) - stmt->options |= CLUOPT_VERBOSE; + options |= CLUOPT_VERBOSE; else - stmt->options &= ~CLUOPT_VERBOSE; + options &= ~CLUOPT_VERBOSE; else ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -194,7 +195,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) table_close(rel, NoLock); /* Do the job. */ - cluster_rel(tableOid, indexOid, stmt->options, isTopLevel); + cluster_rel(tableOid, indexOid, options, isTopLevel); } else { @@ -243,7 +244,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) PushActiveSnapshot(GetTransactionSnapshot()); /* Do the job. */ cluster_rel(rvtc->tableOid, rvtc->indexOid, - stmt->options | CLUOPT_RECHECK, + options | CLUOPT_RECHECK, isTopLevel); PopActiveSnapshot(); CommitTransactionCommand(); diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index e8383feea7..209bbda65b 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2420,13 +2420,14 @@ ChooseIndexColumnNames(List *indexElems) * Recreate a specific index. */ void -ReindexIndex(ReindexStmt *stmt) +ReindexIndex(ReindexStmt *stmt, int options) { RangeVar *indexRelation = stmt->relation; struct ReindexIndexCallbackState state; Oid indOid; Relation irel; char persistence; + bool concurrent = (options & REINDEXOPT_CONCURRENT); /* * Find and lock index, and check permissions on table; use callback to @@ -2438,10 +2439,10 @@ ReindexIndex(ReindexStmt *stmt) * upgrade the lock, but that's OK, because other sessions can't hold * locks on our temporary table. */ - state.concurrent = stmt->concurrent; + state.concurrent = concurrent; state.locked_table_oid = InvalidOid; indOid = RangeVarGetRelidExtended(indexRelation, - stmt->concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock, + concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock, 0, RangeVarCallbackForReindexIndex, &state); @@ -2461,11 +2462,11 @@ ReindexIndex(ReindexStmt *stmt) persistence = irel->rd_rel->relpersistence; index_close(irel, NoLock); - if (stmt->concurrent && persistence != RELPERSISTENCE_TEMP) - ReindexRelationConcurrently(indOid, stmt->options); + if (concurrent && persistence != RELPERSISTENCE_TEMP) + ReindexRelationConcurrently(indOid, options); else reindex_index(indOid, false, persistence, - stmt->options | REINDEXOPT_REPORT_PROGRESS); + options | REINDEXOPT_REPORT_PROGRESS); } /* @@ -2543,11 +2544,12 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation, * Recreate all indexes of a table (and of its toast table, if any) */ Oid -ReindexTable(ReindexStmt *stmt) +ReindexTable(ReindexStmt *stmt, int options) { RangeVar *relation = stmt->relation; Oid heapOid; bool result; + bool concurrent = (options & REINDEXOPT_CONCURRENT); /* * The lock level used here should match reindex_relation(). @@ -2558,13 +2560,13 @@ ReindexTable(ReindexStmt *stmt) * locks on our temporary table. */ heapOid = RangeVarGetRelidExtended(relation, - stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock, + concurrent ? ShareUpdateExclusiveLock : ShareLock, 0, RangeVarCallbackOwnsTable, NULL); - if (stmt->concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP) + if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP) { - result = ReindexRelationConcurrently(heapOid, stmt->options); + result = ReindexRelationConcurrently(heapOid, options); if (!result) ereport(NOTICE, @@ -2576,7 +2578,7 @@ ReindexTable(ReindexStmt *stmt) result = reindex_relation(heapOid, REINDEX_REL_PROCESS_TOAST | REINDEX_REL_CHECK_CONSTRAINTS, - stmt->options | REINDEXOPT_REPORT_PROGRESS); + options | REINDEXOPT_REPORT_PROGRESS); if (!result) ereport(NOTICE, (errmsg("table \"%s\" has no indexes to reindex", @@ -2595,7 +2597,7 @@ ReindexTable(ReindexStmt *stmt) * That means this must not be called within a user transaction block! */ void -ReindexMultipleTables(ReindexStmt *stmt) +ReindexMultipleTables(ReindexStmt *stmt, int options) { const char *objectName = stmt->name; ReindexObjectType objectKind = stmt->kind; @@ -2610,13 +2612,14 @@ ReindexMultipleTables(ReindexStmt *stmt) ListCell *l; int num_keys; bool concurrent_warning = false; + bool concurrent = (options & REINDEXOPT_CONCURRENT); AssertArg(objectName); Assert(objectKind == REINDEX_OBJECT_SCHEMA || objectKind == REINDEX_OBJECT_SYSTEM || objectKind == REINDEX_OBJECT_DATABASE); - if (objectKind == REINDEX_OBJECT_SYSTEM && stmt->concurrent) + if (objectKind == REINDEX_OBJECT_SYSTEM && concurrent) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot reindex system catalogs concurrently"))); @@ -2727,7 +2730,7 @@ ReindexMultipleTables(ReindexStmt *stmt) * Skip system tables, since index_create() would reject indexing them * concurrently (and it would likely fail if we tried). */ - if (stmt->concurrent && + if (concurrent && IsCatalogRelationOid(relid)) { if (!concurrent_warning) @@ -2777,10 +2780,10 @@ ReindexMultipleTables(ReindexStmt *stmt) continue; } - if (stmt->concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP) + if (concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP) { (void) ReindexRelationConcurrently(relid, - stmt->options | + options | REINDEXOPT_MISSING_OK); /* ReindexRelationConcurrently() does the verbose output */ } @@ -2791,11 +2794,11 @@ ReindexMultipleTables(ReindexStmt *stmt) result = reindex_relation(relid, REINDEX_REL_PROCESS_TOAST | REINDEX_REL_CHECK_CONSTRAINTS, - stmt->options | + options | REINDEXOPT_REPORT_PROGRESS | REINDEXOPT_MISSING_OK); - if (result && (stmt->options & REINDEXOPT_VERBOSE)) + if (result && (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 71548acc0c..2a73f1cb6d 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -3357,7 +3357,6 @@ _copyClusterStmt(const ClusterStmt *from) COPY_NODE_FIELD(relation); COPY_STRING_FIELD(indexname); - COPY_SCALAR_FIELD(options); COPY_NODE_FIELD(params); return newnode; @@ -4450,9 +4449,7 @@ _copyReindexStmt(const ReindexStmt *from) COPY_SCALAR_FIELD(kind); 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 de48a42cdd..3c6c40a50d 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -1216,7 +1216,6 @@ _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,9 +2134,7 @@ _equalReindexStmt(const ReindexStmt *a, const ReindexStmt *b) COMPARE_SCALAR_FIELD(kind); 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 c5be94af45..525dfb8132 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -8176,40 +8176,48 @@ ReindexStmt: { ReindexStmt *n = makeNode(ReindexStmt); n->kind = $2; - n->concurrent = $3; n->relation = $4; n->name = NULL; n->params = NIL; + if ($3) + n->params = lappend(n->params, + makeDefElem("concurrently", (Node *)makeString("concurrently"), @3)); $$ = (Node *)n; } | REINDEX reindex_target_multitable opt_concurrently name { ReindexStmt *n = makeNode(ReindexStmt); n->kind = $2; - n->concurrent = $3; n->name = $4; n->relation = NULL; n->params = NIL; + if ($3) + n->params = lappend(n->params, + makeDefElem("concurrently", (Node *)makeString("concurrently"), @3)); $$ = (Node *)n; } | 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->params = $3; + if ($6) + n->params = lappend(n->params, + makeDefElem("concurrently", (Node *)makeString("concurrently"), @6)); $$ = (Node *)n; } | 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->params = $3; + if ($6) + n->params = lappend(n->params, + makeDefElem("concurrently", (Node *)makeString("concurrently"), @6)); $$ = (Node *)n; } ; @@ -10384,10 +10392,9 @@ ClusterStmt: ClusterStmt *n = makeNode(ClusterStmt); n->relation = $3; n->indexname = $4; - n->options = 0; - if ($2) - n->options |= CLUOPT_VERBOSE; n->params = NIL; + if ($2) + n->params = lappend(n->params, makeDefElem("verbose", NULL, @2)); $$ = (Node*)n; } @@ -10404,10 +10411,9 @@ ClusterStmt: ClusterStmt *n = makeNode(ClusterStmt); n->relation = NULL; n->indexname = NULL; - n->options = 0; - if ($2) - n->options |= CLUOPT_VERBOSE; n->params = NIL; + if ($2) + n->params = lappend(n->params, makeDefElem("verbose", NULL, @2)); $$ = (Node*)n; } /* kept for pre-8.3 compatibility */ @@ -10416,10 +10422,9 @@ ClusterStmt: ClusterStmt *n = makeNode(ClusterStmt); n->relation = $5; n->indexname = $3; - n->options = 0; - if ($2) - n->options |= CLUOPT_VERBOSE; n->params = NIL; + if ($2) + n->params = lappend(n->params, makeDefElem("verbose", NULL, @2)); $$ = (Node*)n; } ; diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 0e074d7745..2f66e198f7 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -528,7 +528,7 @@ ProcessUtility(PlannedStmt *pstmt, /* Parse params not parsed by the grammar */ static -void parse_reindex_params(ParseState *pstate, ReindexStmt *stmt) +void parse_reindex_params(ParseState *pstate, ReindexStmt *stmt, int *options) { ListCell *lc; foreach(lc, stmt->params) @@ -538,12 +538,15 @@ void parse_reindex_params(ParseState *pstate, ReindexStmt *stmt) if (strcmp(opt->defname, "verbose") == 0) { if (defGetBoolean(opt)) - stmt->options |= REINDEXOPT_VERBOSE; + *options |= REINDEXOPT_VERBOSE; else - stmt->options &= ~REINDEXOPT_VERBOSE; + *options &= ~REINDEXOPT_VERBOSE; } else if (strcmp(opt->defname, "concurrently") == 0) - stmt->concurrent = true; + if (defGetBoolean(opt)) + *options |= REINDEXOPT_CONCURRENT; + else + *options &= ~REINDEXOPT_CONCURRENT; else ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -945,19 +948,20 @@ standard_ProcessUtility(PlannedStmt *pstmt, case T_ReindexStmt: { ReindexStmt *stmt = (ReindexStmt *) parsetree; + int options = 0; - if (stmt->concurrent) + parse_reindex_params(pstate, stmt, &options); + if (options & REINDEXOPT_CONCURRENT) PreventInTransactionBlock(isTopLevel, "REINDEX CONCURRENTLY"); - parse_reindex_params(pstate, stmt); switch (stmt->kind) { case REINDEX_OBJECT_INDEX: - ReindexIndex(stmt); + ReindexIndex(stmt, options); break; case REINDEX_OBJECT_TABLE: - ReindexTable(stmt); + ReindexTable(stmt, options); break; case REINDEX_OBJECT_SCHEMA: case REINDEX_OBJECT_SYSTEM: @@ -973,7 +977,7 @@ standard_ProcessUtility(PlannedStmt *pstmt, (stmt->kind == REINDEX_OBJECT_SCHEMA) ? "REINDEX SCHEMA" : (stmt->kind == REINDEX_OBJECT_SYSTEM) ? "REINDEX SYSTEM" : "REINDEX DATABASE"); - ReindexMultipleTables(stmt); + ReindexMultipleTables(stmt, options); break; default: elog(ERROR, "unrecognized object type: %d", diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h index a97c00a02e..fedbbc17e0 100644 --- a/src/include/commands/defrem.h +++ b/src/include/commands/defrem.h @@ -34,9 +34,9 @@ extern ObjectAddress DefineIndex(Oid relationId, bool check_not_in_use, bool skip_build, bool quiet); -extern void ReindexIndex(ReindexStmt *stmt); -extern Oid ReindexTable(ReindexStmt *stmt); -extern void ReindexMultipleTables(ReindexStmt *stmt); +extern void ReindexIndex(ReindexStmt *stmt, int options); +extern Oid ReindexTable(ReindexStmt *stmt, int options); +extern void ReindexMultipleTables(ReindexStmt *stmt, int options); 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 4e3188ff87..a524402b91 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -3210,8 +3210,7 @@ typedef struct ClusterStmt NodeTag type; 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 */ + List *params; /* list of DefElem nodes */ } ClusterStmt; /* ---------------------- @@ -3354,6 +3353,7 @@ typedef struct ConstraintsSetStmt #define REINDEXOPT_VERBOSE (1 << 0) /* print progress info */ #define REINDEXOPT_REPORT_PROGRESS (1 << 1) /* report pgstat progress */ #define REINDEXOPT_MISSING_OK (1 << 2) /* skip missing relations */ +#define REINDEXOPT_CONCURRENT (1 << 3) /* reindex CONCURRENTLY */ typedef enum ReindexObjectType { @@ -3371,9 +3371,7 @@ typedef struct ReindexStmt * etc. */ 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? */ + List *params; /* list of DefElem nodes */ } ReindexStmt; /* ---------------------- -- 2.17.0