Hi all, $subject has been mentioned a couple of times, including today: https://www.postgresql.org/message-id/20200902010012.ge1...@paquier.xyz
We have a boolean argument in ReindexStmt to control a concurrent run, and we also have in parallel of that a bitmask to control the options of the statement, which feels like a duplicate. Attached is a patch to refactor the whole, adding CONCURRENTLY as a member of the available options. This simplifies a bit the code. Any thoughts? -- Michael
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index c26a102b17..3129b684f6 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -34,10 +34,10 @@ 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 ReindexIndex(RangeVar *indexRelation, int options);
+extern Oid ReindexTable(RangeVar *relation, int options);
extern void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
- int options, bool concurrent);
+ 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 d52c563305..e83329fd6d 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3353,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_CONCURRENTLY (1 << 3) /* concurrent mode */
typedef enum ReindexObjectType
{
@@ -3371,7 +3372,6 @@ typedef struct ReindexStmt
RangeVar *relation; /* Table or index to reindex */
const char *name; /* name of database to reindex */
int options; /* Reindex options flags */
- bool concurrent; /* reindex concurrently? */
} ReindexStmt;
/* ----------------------
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index b3a92381f9..040185dc54 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -97,7 +97,7 @@ static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
*/
struct ReindexIndexCallbackState
{
- bool concurrent; /* flag from statement */
+ bool options; /* flag from statement */
Oid locked_table_oid; /* tracks previously locked table */
};
@@ -2420,7 +2420,7 @@ ChooseIndexColumnNames(List *indexElems)
* Recreate a specific index.
*/
void
-ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
+ReindexIndex(RangeVar *indexRelation, int options)
{
struct ReindexIndexCallbackState state;
Oid indOid;
@@ -2437,10 +2437,11 @@ 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.options = options;
state.locked_table_oid = InvalidOid;
indOid = RangeVarGetRelidExtended(indexRelation,
- concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock,
+ (options & REINDEXOPT_CONCURRENTLY) != 0 ?
+ ShareUpdateExclusiveLock : AccessExclusiveLock,
0,
RangeVarCallbackForReindexIndex,
&state);
@@ -2460,7 +2461,8 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
persistence = irel->rd_rel->relpersistence;
index_close(irel, NoLock);
- if (concurrent && persistence != RELPERSISTENCE_TEMP)
+ if ((options & REINDEXOPT_CONCURRENTLY) != 0 &&
+ persistence != RELPERSISTENCE_TEMP)
ReindexRelationConcurrently(indOid, options);
else
reindex_index(indOid, false, persistence,
@@ -2485,7 +2487,8 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
* non-concurrent case and table locks used by index_concurrently_*() for
* concurrent case.
*/
- table_lockmode = state->concurrent ? ShareUpdateExclusiveLock : ShareLock;
+ table_lockmode = ((state->options & REINDEXOPT_CONCURRENTLY) != 0) ?
+ ShareUpdateExclusiveLock : ShareLock;
/*
* If we previously locked some other index's heap, and the name we're
@@ -2542,7 +2545,7 @@ 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(RangeVar *relation, int options)
{
Oid heapOid;
bool result;
@@ -2556,11 +2559,13 @@ ReindexTable(RangeVar *relation, int options, bool concurrent)
* locks on our temporary table.
*/
heapOid = RangeVarGetRelidExtended(relation,
- concurrent ? ShareUpdateExclusiveLock : ShareLock,
+ (options & REINDEXOPT_CONCURRENTLY) != 0 ?
+ ShareUpdateExclusiveLock : ShareLock,
0,
RangeVarCallbackOwnsTable, NULL);
- if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
+ if ((options & REINDEXOPT_CONCURRENTLY) != 0 &&
+ get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
{
result = ReindexRelationConcurrently(heapOid, options);
@@ -2594,7 +2599,7 @@ ReindexTable(RangeVar *relation, int options, bool concurrent)
*/
void
ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
- int options, bool concurrent)
+ int options)
{
Oid objectOid;
Relation relationRelation;
@@ -2613,7 +2618,8 @@ 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 &&
+ (options & REINDEXOPT_CONCURRENTLY) != 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot reindex system catalogs concurrently")));
@@ -2724,7 +2730,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 ((options & REINDEXOPT_CONCURRENTLY) != 0 &&
IsCatalogRelationOid(relid))
{
if (!concurrent_warning)
@@ -2774,7 +2780,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
continue;
}
- if (concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP)
+ if ((options & REINDEXOPT_CONCURRENTLY) != 0 &&
+ get_rel_persistence(relid) != RELPERSISTENCE_TEMP)
{
(void) ReindexRelationConcurrently(relid,
options |
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 89c409de66..0409a40b82 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -4450,7 +4450,6 @@ _copyReindexStmt(const ReindexStmt *from)
COPY_NODE_FIELD(relation);
COPY_STRING_FIELD(name);
COPY_SCALAR_FIELD(options);
- COPY_SCALAR_FIELD(concurrent);
return newnode;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index e3f33c40be..e2d1b987bf 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2135,7 +2135,6 @@ _equalReindexStmt(const ReindexStmt *a, const ReindexStmt *b)
COMPARE_NODE_FIELD(relation);
COMPARE_STRING_FIELD(name);
COMPARE_SCALAR_FIELD(options);
- COMPARE_SCALAR_FIELD(concurrent);
return true;
}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index dbb47d4982..c5154b818c 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -8177,40 +8177,44 @@ ReindexStmt:
{
ReindexStmt *n = makeNode(ReindexStmt);
n->kind = $2;
- n->concurrent = $3;
n->relation = $4;
n->name = NULL;
n->options = 0;
+ if ($3)
+ n->options |= REINDEXOPT_CONCURRENTLY;
$$ = (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->options = 0;
+ if ($3)
+ n->options |= REINDEXOPT_CONCURRENTLY;
$$ = (Node *)n;
}
| REINDEX '(' reindex_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;
+ if ($6)
+ n->options |= REINDEXOPT_CONCURRENTLY;
$$ = (Node *)n;
}
| REINDEX '(' reindex_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;
+ if ($6)
+ n->options |= REINDEXOPT_CONCURRENTLY;
$$ = (Node *)n;
}
;
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 6154d2c8c6..b4cde5565e 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -919,17 +919,17 @@ standard_ProcessUtility(PlannedStmt *pstmt,
{
ReindexStmt *stmt = (ReindexStmt *) parsetree;
- if (stmt->concurrent)
+ if ((stmt->options & REINDEXOPT_CONCURRENTLY) != 0)
PreventInTransactionBlock(isTopLevel,
"REINDEX CONCURRENTLY");
switch (stmt->kind)
{
case REINDEX_OBJECT_INDEX:
- ReindexIndex(stmt->relation, stmt->options, stmt->concurrent);
+ ReindexIndex(stmt->relation, stmt->options);
break;
case REINDEX_OBJECT_TABLE:
- ReindexTable(stmt->relation, stmt->options, stmt->concurrent);
+ ReindexTable(stmt->relation, stmt->options);
break;
case REINDEX_OBJECT_SCHEMA:
case REINDEX_OBJECT_SYSTEM:
@@ -945,7 +945,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->name, stmt->kind, stmt->options);
break;
default:
elog(ERROR, "unrecognized object type: %d",
signature.asc
Description: PGP signature
