On 2020-12-11 21:27, Alvaro Herrera wrote:
By the way-- What did you think of the idea of explictly marking the
types used for bitmasks using types bits32 and friends, instead of plain
int, which is harder to spot?
If we want to make it clearer, why not turn the thing into a struct, as
in the attached patch, and avoid the bit fiddling altogether.
From d3125b3bfb8a3dc23e38f38bcf850ca6fc36f492 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Sat, 12 Dec 2020 09:17:55 +0100
Subject: [PATCH] Convert reindex options to struct
---
src/backend/catalog/index.c | 19 ++++---
src/backend/commands/cluster.c | 2 +-
src/backend/commands/indexcmds.c | 94 ++++++++++++++++----------------
src/backend/commands/tablecmds.c | 2 +-
src/backend/tcop/utility.c | 4 +-
src/include/catalog/index.h | 16 +++---
src/include/commands/defrem.h | 9 +--
7 files changed, 74 insertions(+), 72 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 731610c701..06342fddf1 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3594,7 +3594,7 @@ IndexGetRelation(Oid indexId, bool missing_ok)
*/
void
reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
- int options)
+ ReindexOptions options)
{
Relation iRel,
heapRelation;
@@ -3602,7 +3602,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks,
char persistence,
IndexInfo *indexInfo;
volatile bool skipped_constraint = false;
PGRUsage ru0;
- bool progress = (options & REINDEXOPT_REPORT_PROGRESS) != 0;
+ bool progress = options.REINDEXOPT_REPORT_PROGRESS;
pg_rusage_init(&ru0);
@@ -3611,12 +3611,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks,
char persistence,
* we only need to be sure no schema or data changes are going on.
*/
heapId = IndexGetRelation(indexId,
- (options &
REINDEXOPT_MISSING_OK) != 0);
+
options.REINDEXOPT_MISSING_OK);
/* if relation is missing, leave */
if (!OidIsValid(heapId))
return;
- if ((options & REINDEXOPT_MISSING_OK) != 0)
+ if (options.REINDEXOPT_MISSING_OK)
heapRelation = try_table_open(heapId, ShareLock);
else
heapRelation = table_open(heapId, ShareLock);
@@ -3792,7 +3792,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks,
char persistence,
}
/* Log what we did */
- if (options & REINDEXOPT_VERBOSE)
+ if (options.REINDEXOPT_VERBOSE)
ereport(INFO,
(errmsg("index \"%s\" was reindexed",
get_rel_name(indexId)),
@@ -3846,7 +3846,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks,
char persistence,
* index rebuild.
*/
bool
-reindex_relation(Oid relid, int flags, int options)
+reindex_relation(Oid relid, int flags, ReindexOptions options)
{
Relation rel;
Oid toast_relid;
@@ -3861,7 +3861,7 @@ reindex_relation(Oid relid, int flags, int options)
* to prevent schema and data changes in it. The lock level used here
* should match ReindexTable().
*/
- if ((options & REINDEXOPT_MISSING_OK) != 0)
+ if (options.REINDEXOPT_MISSING_OK)
rel = try_table_open(relid, ShareLock);
else
rel = table_open(relid, ShareLock);
@@ -3965,8 +3965,9 @@ reindex_relation(Oid relid, int flags, int options)
* Note that this should fail if the toast relation is missing,
so
* reset REINDEXOPT_MISSING_OK.
*/
- result |= reindex_relation(toast_relid, flags,
- options &
~(REINDEXOPT_MISSING_OK));
+ ReindexOptions newoptions = options;
+ newoptions.REINDEXOPT_MISSING_OK = false;
+ result |= reindex_relation(toast_relid, flags, newoptions);
}
return result;
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index fd5a6eec86..b0aa3536d1 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1412,7 +1412,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
PROGRESS_CLUSTER_PHASE_REBUILD_INDEX);
- reindex_relation(OIDOldHeap, reindex_flags, 0);
+ reindex_relation(OIDOldHeap, reindex_flags, (ReindexOptions){});
/* Report that we are now doing clean up */
pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 14d24b3cc4..cd8eaa732d 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -89,9 +89,9 @@ static List *ChooseIndexColumnNames(List *indexElems);
static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
static void reindex_error_callback(void *args);
-static void ReindexPartitions(Oid relid, int options, bool isTopLevel);
-static void ReindexMultipleInternal(List *relids, int options);
-static bool ReindexRelationConcurrently(Oid relationOid, int options);
+static void ReindexPartitions(Oid relid, ReindexOptions options, bool
isTopLevel);
+static void ReindexMultipleInternal(List *relids, ReindexOptions options);
+static bool ReindexRelationConcurrently(Oid relationOid, ReindexOptions
options);
static void update_relispartition(Oid relationId, bool newval);
static inline void set_indexsafe_procflags(void);
@@ -100,7 +100,7 @@ static inline void set_indexsafe_procflags(void);
*/
struct ReindexIndexCallbackState
{
- int options; /* options from
statement */
+ ReindexOptions options; /* options from statement */
Oid locked_table_oid; /* tracks previously
locked table */
};
@@ -2455,13 +2455,11 @@ ChooseIndexColumnNames(List *indexElems)
* ReindexParseOptions
* Parse list of REINDEX options, returning a bitmask of
ReindexOption.
*/
-int
+ReindexOptions
ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt)
{
ListCell *lc;
- int options = 0;
- bool concurrently = false;
- bool verbose = false;
+ ReindexOptions options = {};
/* Parse option list */
foreach(lc, stmt->params)
@@ -2469,9 +2467,9 @@ ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt)
DefElem *opt = (DefElem *) lfirst(lc);
if (strcmp(opt->defname, "verbose") == 0)
- verbose = defGetBoolean(opt);
+ options.REINDEXOPT_VERBOSE = defGetBoolean(opt);
else if (strcmp(opt->defname, "concurrently") == 0)
- concurrently = defGetBoolean(opt);
+ options.REINDEXOPT_CONCURRENTLY = defGetBoolean(opt);
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -2480,10 +2478,6 @@ ReindexParseOptions(ParseState *pstate, ReindexStmt
*stmt)
parser_errposition(pstate,
opt->location)));
}
- options =
- (verbose ? REINDEXOPT_VERBOSE : 0) |
- (concurrently ? REINDEXOPT_CONCURRENTLY : 0);
-
return options;
}
@@ -2492,7 +2486,7 @@ ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt)
* Recreate a specific index.
*/
void
-ReindexIndex(RangeVar *indexRelation, int options, bool isTopLevel)
+ReindexIndex(RangeVar *indexRelation, ReindexOptions options, bool isTopLevel)
{
struct ReindexIndexCallbackState state;
Oid indOid;
@@ -2512,7 +2506,7 @@ ReindexIndex(RangeVar *indexRelation, int options, bool
isTopLevel)
state.options = options;
state.locked_table_oid = InvalidOid;
indOid = RangeVarGetRelidExtended(indexRelation,
-
(options & REINDEXOPT_CONCURRENTLY) != 0 ?
+
options.REINDEXOPT_CONCURRENTLY ?
ShareUpdateExclusiveLock : AccessExclusiveLock,
0,
RangeVarCallbackForReindexIndex,
@@ -2527,12 +2521,15 @@ ReindexIndex(RangeVar *indexRelation, int options, bool
isTopLevel)
if (relkind == RELKIND_PARTITIONED_INDEX)
ReindexPartitions(indOid, options, isTopLevel);
- else if ((options & REINDEXOPT_CONCURRENTLY) != 0 &&
+ else if (options.REINDEXOPT_CONCURRENTLY &&
persistence != RELPERSISTENCE_TEMP)
ReindexRelationConcurrently(indOid, options);
else
- reindex_index(indOid, false, persistence,
- options | REINDEXOPT_REPORT_PROGRESS);
+ {
+ ReindexOptions newoptions = options;
+ newoptions.REINDEXOPT_REPORT_PROGRESS = true;
+ reindex_index(indOid, false, persistence, newoptions);
+ }
}
/*
@@ -2553,7 +2550,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
* non-concurrent case and table locks used by index_concurrently_*()
for
* concurrent case.
*/
- table_lockmode = ((state->options & REINDEXOPT_CONCURRENTLY) != 0) ?
+ table_lockmode = state->options.REINDEXOPT_CONCURRENTLY ?
ShareUpdateExclusiveLock : ShareLock;
/*
@@ -2611,7 +2608,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 isTopLevel)
+ReindexTable(RangeVar *relation, ReindexOptions options, bool isTopLevel)
{
Oid heapOid;
bool result;
@@ -2625,14 +2622,14 @@ ReindexTable(RangeVar *relation, int options, bool
isTopLevel)
* locks on our temporary table.
*/
heapOid = RangeVarGetRelidExtended(relation,
-
(options & REINDEXOPT_CONCURRENTLY) != 0 ?
+
options.REINDEXOPT_CONCURRENTLY ?
ShareUpdateExclusiveLock : ShareLock,
0,
RangeVarCallbackOwnsTable, NULL);
if (get_rel_relkind(heapOid) == RELKIND_PARTITIONED_TABLE)
ReindexPartitions(heapOid, options, isTopLevel);
- else if ((options & REINDEXOPT_CONCURRENTLY) != 0 &&
+ else if (options.REINDEXOPT_CONCURRENTLY &&
get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
{
result = ReindexRelationConcurrently(heapOid, options);
@@ -2644,10 +2641,12 @@ ReindexTable(RangeVar *relation, int options, bool
isTopLevel)
}
else
{
+ ReindexOptions newoptions = options;
+ newoptions.REINDEXOPT_REPORT_PROGRESS = true;
result = reindex_relation(heapOid,
REINDEX_REL_PROCESS_TOAST |
REINDEX_REL_CHECK_CONSTRAINTS,
- options |
REINDEXOPT_REPORT_PROGRESS);
+ newoptions);
if (!result)
ereport(NOTICE,
(errmsg("table \"%s\" has no indexes to
reindex",
@@ -2667,7 +2666,7 @@ ReindexTable(RangeVar *relation, int options, bool
isTopLevel)
*/
void
ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
- int options)
+ ReindexOptions options)
{
Oid objectOid;
Relation relationRelation;
@@ -2686,7 +2685,7 @@ ReindexMultipleTables(const char *objectName,
ReindexObjectType objectKind,
objectKind == REINDEX_OBJECT_DATABASE);
if (objectKind == REINDEX_OBJECT_SYSTEM &&
- (options & REINDEXOPT_CONCURRENTLY) != 0)
+ options.REINDEXOPT_CONCURRENTLY)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot reindex system catalogs
concurrently")));
@@ -2794,7 +2793,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 ((options & REINDEXOPT_CONCURRENTLY) != 0 &&
+ if (options.REINDEXOPT_CONCURRENTLY &&
IsCatalogRelationOid(relid))
{
if (!concurrent_warning)
@@ -2860,7 +2859,7 @@ reindex_error_callback(void *arg)
* by the caller.
*/
static void
-ReindexPartitions(Oid relid, int options, bool isTopLevel)
+ReindexPartitions(Oid relid, ReindexOptions options, bool isTopLevel)
{
List *partitions = NIL;
char relkind = get_rel_relkind(relid);
@@ -2955,7 +2954,7 @@ ReindexPartitions(Oid relid, int options, bool isTopLevel)
* and starts a new transaction when finished.
*/
static void
-ReindexMultipleInternal(List *relids, int options)
+ReindexMultipleInternal(List *relids, ReindexOptions options)
{
ListCell *l;
@@ -2991,35 +2990,36 @@ ReindexMultipleInternal(List *relids, int options)
Assert(relkind != RELKIND_PARTITIONED_INDEX &&
relkind != RELKIND_PARTITIONED_TABLE);
- if ((options & REINDEXOPT_CONCURRENTLY) != 0 &&
+ if (options.REINDEXOPT_CONCURRENTLY &&
relpersistence != RELPERSISTENCE_TEMP)
{
- (void) ReindexRelationConcurrently(relid,
-
options |
-
REINDEXOPT_MISSING_OK);
+ ReindexOptions newoptions = options;
+ newoptions.REINDEXOPT_MISSING_OK = true;
+ (void) ReindexRelationConcurrently(relid, newoptions);
/* ReindexRelationConcurrently() does the verbose
output */
}
else if (relkind == RELKIND_INDEX)
{
- reindex_index(relid, false, relpersistence,
- options |
- REINDEXOPT_REPORT_PROGRESS |
- REINDEXOPT_MISSING_OK);
+ ReindexOptions newoptions = options;
+ newoptions.REINDEXOPT_REPORT_PROGRESS = true;
+ newoptions.REINDEXOPT_MISSING_OK = true;
+ reindex_index(relid, false, relpersistence, newoptions);
PopActiveSnapshot();
/* reindex_index() does the verbose output */
}
else
{
bool result;
+ ReindexOptions newoptions = options;
+ newoptions.REINDEXOPT_REPORT_PROGRESS = true;
+ newoptions.REINDEXOPT_MISSING_OK = true;
result = reindex_relation(relid,
REINDEX_REL_PROCESS_TOAST |
REINDEX_REL_CHECK_CONSTRAINTS,
-
options |
-
REINDEXOPT_REPORT_PROGRESS |
-
REINDEXOPT_MISSING_OK);
+
newoptions);
- if (result && (options & REINDEXOPT_VERBOSE))
+ if (result && options.REINDEXOPT_VERBOSE)
ereport(INFO,
(errmsg("table \"%s.%s\" was
reindexed",
get_namespace_name(get_rel_namespace(relid)),
@@ -3059,7 +3059,7 @@ ReindexMultipleInternal(List *relids, int options)
* anyway, and a non-concurrent reindex is more efficient.
*/
static bool
-ReindexRelationConcurrently(Oid relationOid, int options)
+ReindexRelationConcurrently(Oid relationOid, ReindexOptions options)
{
List *heapRelationIds = NIL;
List *indexIds = NIL;
@@ -3092,7 +3092,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
"ReindexConcurrent",
ALLOCSET_SMALL_SIZES);
- if (options & REINDEXOPT_VERBOSE)
+ if (options.REINDEXOPT_VERBOSE)
{
/* Save data needed by REINDEX VERBOSE in private context */
oldcontext = MemoryContextSwitchTo(private_context);
@@ -3137,7 +3137,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
errmsg("cannot reindex
system catalogs concurrently")));
/* Open relation to get its indexes */
- if ((options & REINDEXOPT_MISSING_OK) != 0)
+ if (options.REINDEXOPT_MISSING_OK)
{
heapRelation =
try_table_open(relationOid,
ShareUpdateExclusiveLock);
@@ -3233,7 +3233,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
case RELKIND_INDEX:
{
Oid heapId =
IndexGetRelation(relationOid,
-
(options & REINDEXOPT_MISSING_OK) != 0);
+
options.REINDEXOPT_MISSING_OK);
Relation heapRelation;
/* if relation is missing, leave */
@@ -3262,7 +3262,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
* to rebuild is not complete yet, and
REINDEXOPT_MISSING_OK
* should not be used once all the session
locks are taken.
*/
- if ((options & REINDEXOPT_MISSING_OK) != 0)
+ if (options.REINDEXOPT_MISSING_OK)
{
heapRelation = try_table_open(heapId,
ShareUpdateExclusiveLock);
@@ -3754,7 +3754,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
StartTransactionCommand();
/* Log what we did */
- if (options & REINDEXOPT_VERBOSE)
+ if (options.REINDEXOPT_VERBOSE)
{
if (relkind == RELKIND_INDEX)
ereport(INFO,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1fa9f19f08..9f218ac0e4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1891,7 +1891,7 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids,
List *relids_logged,
/*
* Reconstruct the indexes to match, and we're done.
*/
- reindex_relation(heap_relid, REINDEX_REL_PROCESS_TOAST,
0);
+ reindex_relation(heap_relid, REINDEX_REL_PROCESS_TOAST,
(ReindexOptions){});
}
pgstat_count_truncate(rel);
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index a42ead7d69..e5a4e8f662 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -919,10 +919,10 @@ standard_ProcessUtility(PlannedStmt *pstmt,
case T_ReindexStmt:
{
ReindexStmt *stmt = (ReindexStmt *) parsetree;
- int options;
+ ReindexOptions options;
options = ReindexParseOptions(pstate, stmt);
- if ((options & REINDEXOPT_CONCURRENTLY) != 0)
+ if (options.REINDEXOPT_CONCURRENTLY)
PreventInTransactionBlock(isTopLevel,
"REINDEX CONCURRENTLY");
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index c041628049..81e3de4d22 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -30,13 +30,13 @@ typedef enum
} IndexStateFlagsAction;
/* options for REINDEX */
-typedef enum ReindexOption
+typedef struct ReindexOptions
{
- REINDEXOPT_VERBOSE = 1 << 0, /* print progress info */
- REINDEXOPT_REPORT_PROGRESS = 1 << 1, /* report pgstat progress */
- REINDEXOPT_MISSING_OK = 1 << 2, /* skip missing relations */
- REINDEXOPT_CONCURRENTLY = 1 << 3 /* concurrent mode */
-} ReindexOption;
+ bool REINDEXOPT_VERBOSE; /* print progress info
*/
+ bool REINDEXOPT_REPORT_PROGRESS; /* report pgstat progress */
+ bool REINDEXOPT_MISSING_OK; /* skip missing
relations */
+ bool REINDEXOPT_CONCURRENTLY; /* concurrent mode */
+} ReindexOptions;
/* state info for validate_index bulkdelete callback */
typedef struct ValidateIndexState
@@ -146,7 +146,7 @@ extern void index_set_state_flags(Oid indexId,
IndexStateFlagsAction action);
extern Oid IndexGetRelation(Oid indexId, bool missing_ok);
extern void reindex_index(Oid indexId, bool skip_constraint_checks,
- char relpersistence, int
options);
+ char relpersistence,
ReindexOptions options);
/* Flag bits for reindex_relation(): */
#define REINDEX_REL_PROCESS_TOAST 0x01
@@ -155,7 +155,7 @@ extern void reindex_index(Oid indexId, bool
skip_constraint_checks,
#define REINDEX_REL_FORCE_INDEXES_UNLOGGED 0x08
#define REINDEX_REL_FORCE_INDEXES_PERMANENT 0x10
-extern bool reindex_relation(Oid relid, int flags, int options);
+extern bool reindex_relation(Oid relid, int flags, ReindexOptions options);
extern bool ReindexIsProcessingHeap(Oid heapOid);
extern bool ReindexIsProcessingIndex(Oid indexOid);
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 1133ae1143..5b1a60d5fa 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -14,6 +14,7 @@
#ifndef DEFREM_H
#define DEFREM_H
+#include "catalog/index.h"
#include "catalog/objectaddress.h"
#include "nodes/params.h"
#include "parser/parse_node.h"
@@ -34,11 +35,11 @@ extern ObjectAddress DefineIndex(Oid relationId,
bool
check_not_in_use,
bool
skip_build,
bool quiet);
-extern int ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt);
-extern void ReindexIndex(RangeVar *indexRelation, int options, bool
isTopLevel);
-extern Oid ReindexTable(RangeVar *relation, int options, bool isTopLevel);
+extern ReindexOptions ReindexParseOptions(ParseState *pstate, ReindexStmt
*stmt);
+extern void ReindexIndex(RangeVar *indexRelation, ReindexOptions options, bool
isTopLevel);
+extern Oid ReindexTable(RangeVar *relation, ReindexOptions options, bool
isTopLevel);
extern void ReindexMultipleTables(const char *objectName, ReindexObjectType
objectKind,
- int options);
+
ReindexOptions options);
extern char *makeObjectName(const char *name1, const char *name2,
const char *label);
extern char *ChooseRelationName(const char *name1, const char *name2,
--
2.29.2
