On Tue, Dec 22, 2020 at 06:57:41PM +0900, Michael Paquier wrote: > On Tue, Dec 22, 2020 at 02:32:05AM -0600, Justin Pryzby wrote: > > Also, this one is going to be subsumed by ExecReindex(), so the palloc will > > go > > away (otherwise I would ask to pass it in from the caller): > > Yeah, maybe. Still you need to be very careful if you have any > allocated variables like a tablespace or a path which requires to be > in the private context used by ReindexMultipleInternal() or even > ReindexRelationConcurrently(), so I am not sure you can avoid that > completely. For now, we could choose the option to still use a > palloc(), and then save the options in the private contexts. Forgot > that in the previous version actually.
I can't see why this still uses memset instead of structure assignment. Now, I really think utility.c ought to pass in a pointer to a local ReindexOptions variable to avoid all the memory context, which is unnecessary and prone to error. ExecReindex() will set options.tablesapceOid, not a pointer. Like this. I also changed the callback to be a ReindexOptions and not a pointer. -- Justin
>From f12723a8e4ad550c009935fa5397d1d4a968c7bf Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Tue, 22 Dec 2020 18:57:41 +0900 Subject: [PATCH 1/3] refactor-utility-opts-michael-2.patch - hacked on by Justin --- src/backend/catalog/index.c | 19 ++++--- src/backend/commands/cluster.c | 19 ++++--- src/backend/commands/indexcmds.c | 97 +++++++++++++++++--------------- src/backend/commands/tablecmds.c | 3 +- src/backend/commands/vacuum.c | 6 +- src/backend/tcop/utility.c | 12 ++-- src/include/catalog/index.h | 19 ++++--- src/include/commands/cluster.h | 13 +++-- src/include/commands/defrem.h | 17 ++++-- src/include/commands/vacuum.h | 20 +++---- src/tools/pgindent/typedefs.list | 2 + 11 files changed, 124 insertions(+), 103 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 731610c701..d776c661d7 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->flags & REINDEXOPT_REPORT_PROGRESS) != 0); 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->flags & REINDEXOPT_MISSING_OK) != 0); /* if relation is missing, leave */ if (!OidIsValid(heapId)) return; - if ((options & REINDEXOPT_MISSING_OK) != 0) + if ((options->flags & REINDEXOPT_MISSING_OK) != 0) 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->flags & REINDEXOPT_VERBOSE) != 0) 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->flags & REINDEXOPT_MISSING_OK) != 0) 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.flags &= ~(REINDEXOPT_MISSING_OK); + 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..32f5ae848f 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -103,7 +103,7 @@ void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) { ListCell *lc; - int options = 0; + ClusterOptions options = {0}; bool verbose = false; /* Parse option list */ @@ -121,7 +121,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) parser_errposition(pstate, opt->location))); } - options = (verbose ? CLUOPT_VERBOSE : 0); + options.flags = (verbose ? CLUOPT_VERBOSE : 0); if (stmt->relation != NULL) { @@ -192,7 +192,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) table_close(rel, NoLock); /* Do the job. */ - cluster_rel(tableOid, indexOid, options); + cluster_rel(tableOid, indexOid, &options); } else { @@ -234,6 +234,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) foreach(rv, rvs) { RelToCluster *rvtc = (RelToCluster *) lfirst(rv); + ClusterOptions cluster_options = options; + cluster_options.flags |= CLUOPT_RECHECK; /* Start a new transaction for each relation. */ StartTransactionCommand(); @@ -241,7 +243,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) PushActiveSnapshot(GetTransactionSnapshot()); /* Do the job. */ cluster_rel(rvtc->tableOid, rvtc->indexOid, - options | CLUOPT_RECHECK); + &cluster_options); PopActiveSnapshot(); CommitTransactionCommand(); } @@ -272,11 +274,11 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) * and error messages should refer to the operation as VACUUM not CLUSTER. */ void -cluster_rel(Oid tableOid, Oid indexOid, int options) +cluster_rel(Oid tableOid, Oid indexOid, ClusterOptions *options) { Relation OldHeap; - bool verbose = ((options & CLUOPT_VERBOSE) != 0); - bool recheck = ((options & CLUOPT_RECHECK) != 0); + bool verbose = ((options->flags & CLUOPT_VERBOSE) != 0); + bool recheck = ((options->flags & CLUOPT_RECHECK) != 0); /* Check for user-requested abort. */ CHECK_FOR_INTERRUPTS(); @@ -1355,6 +1357,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, ObjectAddress object; Oid mapped_tables[4]; int reindex_flags; + ReindexOptions reindex_options = {0}; int i; /* Report that we are now swapping relation files */ @@ -1412,7 +1415,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, &reindex_options); /* 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..98a8ade20b 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -89,9 +89,12 @@ 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 +103,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 */ }; @@ -2453,13 +2456,12 @@ ChooseIndexColumnNames(List *indexElems) /* * ReindexParseOptions - * Parse list of REINDEX options, returning a bitmask of ReindexOption. + * Parse stmt into options. */ -int -ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt) +void +ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt, ReindexOptions *options) { ListCell *lc; - int options = 0; bool concurrently = false; bool verbose = false; @@ -2480,11 +2482,9 @@ ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt) parser_errposition(pstate, opt->location))); } - options = + options->flags = (verbose ? REINDEXOPT_VERBOSE : 0) | (concurrently ? REINDEXOPT_CONCURRENTLY : 0); - - return options; } /* @@ -2492,7 +2492,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; @@ -2509,10 +2509,10 @@ ReindexIndex(RangeVar *indexRelation, int options, bool isTopLevel) * upgrade the lock, but that's OK, because other sessions can't hold * locks on our temporary table. */ - state.options = options; + state.options = *options; state.locked_table_oid = InvalidOid; indOid = RangeVarGetRelidExtended(indexRelation, - (options & REINDEXOPT_CONCURRENTLY) != 0 ? + (options->flags & REINDEXOPT_CONCURRENTLY) != 0 ? ShareUpdateExclusiveLock : AccessExclusiveLock, 0, RangeVarCallbackForReindexIndex, @@ -2527,12 +2527,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->flags & REINDEXOPT_CONCURRENTLY) != 0 && persistence != RELPERSISTENCE_TEMP) ReindexRelationConcurrently(indOid, options); else - reindex_index(indOid, false, persistence, - options | REINDEXOPT_REPORT_PROGRESS); + { + ReindexOptions newoptions = *options; + newoptions.flags |= REINDEXOPT_REPORT_PROGRESS; + reindex_index(indOid, false, persistence, &newoptions); + } } /* @@ -2553,7 +2556,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.flags & REINDEXOPT_CONCURRENTLY) != 0 ? ShareUpdateExclusiveLock : ShareLock; /* @@ -2611,7 +2614,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 +2628,14 @@ ReindexTable(RangeVar *relation, int options, bool isTopLevel) * locks on our temporary table. */ heapOid = RangeVarGetRelidExtended(relation, - (options & REINDEXOPT_CONCURRENTLY) != 0 ? + (options->flags & REINDEXOPT_CONCURRENTLY) != 0 ? 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->flags & REINDEXOPT_CONCURRENTLY) != 0 && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP) { result = ReindexRelationConcurrently(heapOid, options); @@ -2644,10 +2647,12 @@ ReindexTable(RangeVar *relation, int options, bool isTopLevel) } else { + ReindexOptions newoptions = *options; + newoptions.flags |= REINDEXOPT_REPORT_PROGRESS; 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 +2672,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 +2691,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, objectKind == REINDEX_OBJECT_DATABASE); if (objectKind == REINDEX_OBJECT_SYSTEM && - (options & REINDEXOPT_CONCURRENTLY) != 0) + (options->flags & REINDEXOPT_CONCURRENTLY) != 0) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot reindex system catalogs concurrently"))); @@ -2794,7 +2799,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->flags & REINDEXOPT_CONCURRENTLY) != 0 && IsCatalogRelationOid(relid)) { if (!concurrent_warning) @@ -2860,7 +2865,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 +2960,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 +2996,35 @@ ReindexMultipleInternal(List *relids, int options) Assert(relkind != RELKIND_PARTITIONED_INDEX && relkind != RELKIND_PARTITIONED_TABLE); - if ((options & REINDEXOPT_CONCURRENTLY) != 0 && + if ((options->flags & REINDEXOPT_CONCURRENTLY) != 0 && relpersistence != RELPERSISTENCE_TEMP) { - (void) ReindexRelationConcurrently(relid, - options | - REINDEXOPT_MISSING_OK); + ReindexOptions newoptions = *options; + newoptions.flags |= REINDEXOPT_MISSING_OK; + (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.flags |= + REINDEXOPT_REPORT_PROGRESS | REINDEXOPT_MISSING_OK; + reindex_index(relid, false, relpersistence, &newoptions); PopActiveSnapshot(); /* reindex_index() does the verbose output */ } else { bool result; - + ReindexOptions newoptions = *options; + newoptions.flags |= + REINDEXOPT_REPORT_PROGRESS | REINDEXOPT_MISSING_OK; 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->flags & REINDEXOPT_VERBOSE) != 0) ereport(INFO, (errmsg("table \"%s.%s\" was reindexed", get_namespace_name(get_rel_namespace(relid)), @@ -3059,7 +3064,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 +3097,7 @@ ReindexRelationConcurrently(Oid relationOid, int options) "ReindexConcurrent", ALLOCSET_SMALL_SIZES); - if (options & REINDEXOPT_VERBOSE) + if ((options->flags & REINDEXOPT_VERBOSE) != 0) { /* Save data needed by REINDEX VERBOSE in private context */ oldcontext = MemoryContextSwitchTo(private_context); @@ -3137,7 +3142,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->flags & REINDEXOPT_MISSING_OK) != 0) { heapRelation = try_table_open(relationOid, ShareUpdateExclusiveLock); @@ -3233,7 +3238,7 @@ ReindexRelationConcurrently(Oid relationOid, int options) case RELKIND_INDEX: { Oid heapId = IndexGetRelation(relationOid, - (options & REINDEXOPT_MISSING_OK) != 0); + (options->flags & REINDEXOPT_MISSING_OK) != 0); Relation heapRelation; /* if relation is missing, leave */ @@ -3262,7 +3267,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->flags & REINDEXOPT_MISSING_OK) != 0) { heapRelation = try_table_open(heapId, ShareUpdateExclusiveLock); @@ -3754,7 +3759,7 @@ ReindexRelationConcurrently(Oid relationOid, int options) StartTransactionCommand(); /* Log what we did */ - if (options & REINDEXOPT_VERBOSE) + if ((options->flags & REINDEXOPT_VERBOSE) != 0) { if (relkind == RELKIND_INDEX) ereport(INFO, diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 1fa9f19f08..7d6b321d75 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -1854,6 +1854,7 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged, { Oid heap_relid; Oid toast_relid; + ReindexOptions reindex_options = {0}; /* * This effectively deletes all rows in the table, and may be done @@ -1891,7 +1892,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, &reindex_options); } pgstat_count_truncate(rel); diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 98270a1049..985450e8f8 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -1916,17 +1916,17 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params) */ if (params->options & VACOPT_FULL) { - int cluster_options = 0; + ClusterOptions cluster_options = {0}; /* close relation before vacuuming, but hold lock until commit */ relation_close(onerel, NoLock); onerel = NULL; if ((params->options & VACOPT_VERBOSE) != 0) - cluster_options |= CLUOPT_VERBOSE; + cluster_options.flags |= CLUOPT_VERBOSE; /* VACUUM FULL is now a variant of CLUSTER; see cluster.c */ - cluster_rel(relid, InvalidOid, cluster_options); + cluster_rel(relid, InvalidOid, &cluster_options); } else table_relation_vacuum(onerel, params, vac_strategy); diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index a42ead7d69..403b46cefb 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -919,20 +919,20 @@ standard_ProcessUtility(PlannedStmt *pstmt, case T_ReindexStmt: { ReindexStmt *stmt = (ReindexStmt *) parsetree; - int options; + ReindexOptions options; - options = ReindexParseOptions(pstate, stmt); - if ((options & REINDEXOPT_CONCURRENTLY) != 0) + ReindexParseOptions(pstate, stmt, &options); + if ((options.flags & REINDEXOPT_CONCURRENTLY) != 0) PreventInTransactionBlock(isTopLevel, "REINDEX CONCURRENTLY"); switch (stmt->kind) { case REINDEX_OBJECT_INDEX: - ReindexIndex(stmt->relation, options, isTopLevel); + ReindexIndex(stmt->relation, &options, isTopLevel); break; case REINDEX_OBJECT_TABLE: - ReindexTable(stmt->relation, options, isTopLevel); + ReindexTable(stmt->relation, &options, isTopLevel); break; case REINDEX_OBJECT_SCHEMA: case REINDEX_OBJECT_SYSTEM: @@ -948,7 +948,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, options); + ReindexMultipleTables(stmt->name, stmt->kind, &options); break; default: elog(ERROR, "unrecognized object type: %d", diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h index c041628049..89394b648e 100644 --- a/src/include/catalog/index.h +++ b/src/include/catalog/index.h @@ -30,13 +30,16 @@ 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; + bits32 flags; /* bitmask of REINDEXOPT_* */ +} ReindexOptions; + +/* flag bits for ReindexOptions->flags */ +#define REINDEXOPT_VERBOSE 0x01 /* print progress info */ +#define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */ +#define REINDEXOPT_MISSING_OK 0x04 /* skip missing relations */ +#define REINDEXOPT_CONCURRENTLY 0x08 /* concurrent mode */ /* state info for validate_index bulkdelete callback */ typedef struct ValidateIndexState @@ -146,7 +149,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 +158,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/cluster.h b/src/include/commands/cluster.h index 7cfb37c9b2..c66629cf73 100644 --- a/src/include/commands/cluster.h +++ b/src/include/commands/cluster.h @@ -18,16 +18,17 @@ #include "storage/lock.h" #include "utils/relcache.h" - /* options for CLUSTER */ -typedef enum ClusterOption +#define CLUOPT_RECHECK 0x01 /* recheck relation state */ +#define CLUOPT_VERBOSE 0x02 /* print progress info */ + +typedef struct ClusterOptions { - CLUOPT_RECHECK = 1 << 0, /* recheck relation state */ - CLUOPT_VERBOSE = 1 << 1 /* print progress info */ -} ClusterOption; + bits32 flags; /* bitmask of CLUSTEROPT_* */ +} ClusterOptions; extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel); -extern void cluster_rel(Oid tableOid, Oid indexOid, int options); +extern void cluster_rel(Oid tableOid, Oid indexOid, ClusterOptions *options); extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMODE lockmode); extern void mark_index_clustered(Relation rel, Oid indexOid, bool is_internal); diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h index 1133ae1143..ca276e222d 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,17 @@ 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 void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, - int options); +extern void ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt, + ReindexOptions *options); +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, + ReindexOptions *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/commands/vacuum.h b/src/include/commands/vacuum.h index a4cd721400..e3fe4b38e8 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -174,17 +174,15 @@ 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; +/* flag bits for VacuumParams->options */ +#define VACOPT_VACUUM 0x01 /* do VACUUM */ +#define VACOPT_ANALYZE 0x02 /* do ANALYZE */ +#define VACOPT_VERBOSE 0x04 /* print progress info */ +#define VACOPT_FREEZE 0x08 /* FREEZE option */ +#define VACOPT_FULL 0x10 /* FULL (non-concurrent) vacuum */ +#define VACOPT_SKIP_LOCKED 0x20 /* skip if cannot get lock */ +#define VACOPT_SKIPTOAST 0x40 /* don't process the TOAST table, if any */ +#define VACOPT_DISABLE_PAGE_SKIPPING 0x80 /* don't skip any pages */ /* * A ternary value used by vacuum parameters. diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index bca37c536e..768f758fa0 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -347,6 +347,7 @@ ClosePortalStmt ClosePtrType Clump ClusterInfo +ClusterOptions ClusterStmt CmdType CoalesceExpr @@ -2050,6 +2051,7 @@ RegisNode RegisteredBgWorker ReindexErrorInfo ReindexObjectType +ReindexOptions ReindexStmt ReindexType RelFileNode -- 2.17.0
>From 03b235487cadd402130848430feabbb3a1abec1f Mon Sep 17 00:00:00 2001 From: Justin Pryzby <pryz...@telsasoft.com> Date: Tue, 22 Dec 2020 14:53:25 -0600 Subject: [PATCH 2/3] ExecReindex --- src/backend/commands/indexcmds.c | 59 ++++++++++++++++++++++++++++---- src/backend/tcop/utility.c | 39 ++------------------- src/include/commands/defrem.h | 12 +------ 3 files changed, 55 insertions(+), 55 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 98a8ade20b..edd394ed4e 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -86,8 +86,14 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId, bool primary, bool isconstraint); static char *ChooseIndexNameAddition(List *colnames); static List *ChooseIndexColumnNames(List *indexElems); +static void ReindexIndex(RangeVar *indexRelation, ReindexOptions *options, + bool isTopLevel); static void RangeVarCallbackForReindexIndex(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); +static Oid ReindexTable(RangeVar *relation, ReindexOptions *options, + bool isTopLevel); +static void ReindexMultipleTables(const char *objectName, + ReindexObjectType objectKind, ReindexOptions *options); static void reindex_error_callback(void *args); static void ReindexPartitions(Oid relid, ReindexOptions *options, bool isTopLevel); @@ -2455,12 +2461,17 @@ ChooseIndexColumnNames(List *indexElems) } /* - * ReindexParseOptions - * Parse stmt into options. + * ExecReindex + * Reindex accordinging to stmt. + * This calls the intermediate routines: ReindexIndex, ReindexTable, ReindexMultipleTables, + * which ultimately call reindex_index, reindex_relation, ReindexRelationConcurrently. + * Note that partitioned relations are handled by ReindexPartitions, except that + * ReindexRelationConcurrently handles concurrently reindexing a table. */ void -ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt, ReindexOptions *options) +ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel) { + ReindexOptions options = {0}; ListCell *lc; bool concurrently = false; bool verbose = false; @@ -2482,16 +2493,50 @@ ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt, ReindexOptions *optio parser_errposition(pstate, opt->location))); } - options->flags = + if (concurrently) + PreventInTransactionBlock(isTopLevel, + "REINDEX CONCURRENTLY"); + + options.flags = (verbose ? REINDEXOPT_VERBOSE : 0) | (concurrently ? REINDEXOPT_CONCURRENTLY : 0); + + switch (stmt->kind) + { + case REINDEX_OBJECT_INDEX: + ReindexIndex(stmt->relation, &options, isTopLevel); + break; + case REINDEX_OBJECT_TABLE: + ReindexTable(stmt->relation, &options, isTopLevel); + break; + case REINDEX_OBJECT_SCHEMA: + case REINDEX_OBJECT_SYSTEM: + case REINDEX_OBJECT_DATABASE: + + /* + * This cannot run inside a user transaction block; if + * we were inside a transaction, then its commit- and + * start-transaction-command calls would not have the + * intended effect! + */ + PreventInTransactionBlock(isTopLevel, + (stmt->kind == REINDEX_OBJECT_SCHEMA) ? "REINDEX SCHEMA" : + (stmt->kind == REINDEX_OBJECT_SYSTEM) ? "REINDEX SYSTEM" : + "REINDEX DATABASE"); + ReindexMultipleTables(stmt->name, stmt->kind, &options); + break; + default: + elog(ERROR, "unrecognized object type: %d", + (int) stmt->kind); + break; + } } /* * ReindexIndex * Recreate a specific index. */ -void +static void ReindexIndex(RangeVar *indexRelation, ReindexOptions *options, bool isTopLevel) { struct ReindexIndexCallbackState state; @@ -2613,7 +2658,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation, * ReindexTable * Recreate all indexes of a table (and of its toast table, if any) */ -Oid +static Oid ReindexTable(RangeVar *relation, ReindexOptions *options, bool isTopLevel) { Oid heapOid; @@ -2670,7 +2715,7 @@ ReindexTable(RangeVar *relation, ReindexOptions *options, bool isTopLevel) * separate transaction, so we can release the lock on it right away. * That means this must not be called within a user transaction block! */ -void +static void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, ReindexOptions *options) { diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 403b46cefb..4942e24a3a 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -919,44 +919,9 @@ standard_ProcessUtility(PlannedStmt *pstmt, case T_ReindexStmt: { ReindexStmt *stmt = (ReindexStmt *) parsetree; - ReindexOptions options; - - ReindexParseOptions(pstate, stmt, &options); - if ((options.flags & REINDEXOPT_CONCURRENTLY) != 0) - PreventInTransactionBlock(isTopLevel, - "REINDEX CONCURRENTLY"); - - switch (stmt->kind) - { - case REINDEX_OBJECT_INDEX: - ReindexIndex(stmt->relation, &options, isTopLevel); - break; - case REINDEX_OBJECT_TABLE: - ReindexTable(stmt->relation, &options, isTopLevel); - break; - case REINDEX_OBJECT_SCHEMA: - case REINDEX_OBJECT_SYSTEM: - case REINDEX_OBJECT_DATABASE: - - /* - * This cannot run inside a user transaction block; if - * we were inside a transaction, then its commit- and - * start-transaction-command calls would not have the - * intended effect! - */ - PreventInTransactionBlock(isTopLevel, - (stmt->kind == REINDEX_OBJECT_SCHEMA) ? "REINDEX SCHEMA" : - (stmt->kind == REINDEX_OBJECT_SYSTEM) ? "REINDEX SYSTEM" : - "REINDEX DATABASE"); - ReindexMultipleTables(stmt->name, stmt->kind, &options); - break; - default: - elog(ERROR, "unrecognized object type: %d", - (int) stmt->kind); - break; - } + ExecReindex(pstate, stmt, isTopLevel); + break; } - break; /* * The following statements are supported by Event Triggers only diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h index ca276e222d..d4ea57e757 100644 --- a/src/include/commands/defrem.h +++ b/src/include/commands/defrem.h @@ -35,17 +35,7 @@ extern ObjectAddress DefineIndex(Oid relationId, bool check_not_in_use, bool skip_build, bool quiet); -extern void ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt, - ReindexOptions *options); -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, - ReindexOptions *options); +extern void ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel); extern char *makeObjectName(const char *name1, const char *name2, const char *label); extern char *ChooseRelationName(const char *name1, const char *name2, -- 2.17.0
>From 899ac88c3310bd33052a8c29564ec5d2f1b6725f Mon Sep 17 00:00:00 2001 From: Alexey Kondratov <kondratov.alek...@gmail.com> Date: Mon, 23 Mar 2020 21:10:29 +0300 Subject: [PATCH 3/3] Allow REINDEX to change tablespace REINDEX already does full relation rewrite, this patch adds a possibility to specify a new tablespace where new relfilenode will be created. --- doc/src/sgml/ref/reindex.sgml | 22 +++++ src/backend/catalog/index.c | 96 +++++++++++++++++++- src/backend/commands/indexcmds.c | 103 +++++++++++++++++++++- src/bin/psql/tab-complete.c | 4 +- src/include/catalog/index.h | 2 + src/test/regress/input/tablespace.source | 53 +++++++++++ src/test/regress/output/tablespace.source | 102 +++++++++++++++++++++ 7 files changed, 377 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 6e1cf06713..e58b1281cd 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -27,6 +27,7 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN CONCURRENTLY [ <replaceable class="parameter">boolean</replaceable> ] VERBOSE [ <replaceable class="parameter">boolean</replaceable> ] + TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> </synopsis> </refsynopsisdiv> @@ -187,6 +188,19 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN </listitem> </varlistentry> + <varlistentry> + <term><literal>TABLESPACE</literal></term> + <listitem> + <para> + This specifies that indexes will be rebuilt on a new tablespace. + Cannot be used with "mapped" relations. If <literal>SCHEMA</literal>, + <literal>DATABASE</literal> or <literal>SYSTEM</literal> is specified, then + all unsuitable relations will be skipped and a single <literal>WARNING</literal> + will be generated. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><literal>VERBOSE</literal></term> <listitem> @@ -210,6 +224,14 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN </listitem> </varlistentry> + <varlistentry> + <term><replaceable class="parameter">new_tablespace</replaceable></term> + <listitem> + <para> + The tablespace where indexes will be rebuilt. + </para> + </listitem> + </varlistentry> </variablelist> </refsect1> diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index d776c661d7..4e5e3f7693 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -57,6 +57,7 @@ #include "commands/event_trigger.h" #include "commands/progress.h" #include "commands/tablecmds.h" +#include "commands/tablespace.h" #include "commands/trigger.h" #include "executor/executor.h" #include "miscadmin.h" @@ -1394,9 +1395,13 @@ index_update_collation_versions(Oid relid, Oid coll) * Create concurrently an index based on the definition of the one provided by * caller. The index is inserted into catalogs and needs to be built later * on. This is called during concurrent reindex processing. + * + * "tablespaceOid" is the new tablespace to use for this index. If + * InvalidOid, use the tablespace in-use instead. */ Oid -index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, const char *newName) +index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, + Oid tablespaceOid, const char *newName) { Relation indexRelation; IndexInfo *oldInfo, @@ -1526,7 +1531,8 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, const char newInfo, indexColNames, indexRelation->rd_rel->relam, - indexRelation->rd_rel->reltablespace, + OidIsValid(tablespaceOid) ? + tablespaceOid : indexRelation->rd_rel->reltablespace, indexRelation->rd_indcollation, indclass->values, indcoloptions->values, @@ -3591,6 +3597,8 @@ IndexGetRelation(Oid indexId, bool missing_ok) /* * reindex_index - This routine is used to recreate a single index + * + * See comments of reindex_relation() for details about "tablespaceOid". */ void reindex_index(Oid indexId, bool skip_constraint_checks, char persistence, @@ -3599,10 +3607,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence, Relation iRel, heapRelation; Oid heapId; + Oid oldTablespaceOid; IndexInfo *indexInfo; volatile bool skipped_constraint = false; PGRUsage ru0; bool progress = ((options->flags & REINDEXOPT_REPORT_PROGRESS) != 0); + bool set_tablespace = OidIsValid(options->tablespaceOid); pg_rusage_init(&ru0); @@ -3654,6 +3664,35 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence, get_namespace_name(RelationGetNamespace(iRel)), RelationGetRelationName(iRel)); + /* + * We don't support moving system relations into different tablespaces + * unless allow_system_table_mods=1. + */ + if (set_tablespace && + !allowSystemTableMods && IsSystemRelation(iRel)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a system catalog", + RelationGetRelationName(iRel)))); + + /* + * We cannot support moving mapped relations into different tablespaces. + * (In particular this eliminates all shared catalogs.) + */ + if (set_tablespace && RelationIsMapped(iRel)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot change tablespace of mapped relation \"%s\"", + RelationGetRelationName(iRel)))); + + /* It's not a shared catalog, so refuse to move it to shared tablespace */ + if (options->tablespaceOid == GLOBALTABLESPACE_OID) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot move non-shared relation to tablespace \"%s\"", + get_tablespace_name(options->tablespaceOid)))); + + /* * Don't allow reindex on temp tables of other backends ... their local * buffer manager is not going to cope. @@ -3680,6 +3719,51 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence, */ CheckTableNotInUse(iRel, "REINDEX INDEX"); + if (options->tablespaceOid == MyDatabaseTableSpace) + options->tablespaceOid = InvalidOid; + + /* + * Set the new tablespace for the relation. Do that only in the + * case where the reindex caller wishes to enforce a new tablespace. + */ + oldTablespaceOid = iRel->rd_rel->reltablespace; + if (set_tablespace && + (options->tablespaceOid != oldTablespaceOid || + (options->tablespaceOid == MyDatabaseTableSpace && OidIsValid(oldTablespaceOid)))) + { + Relation pg_class; + Form_pg_class rd_rel; + HeapTuple tuple; + + /* First get a modifiable copy of the relation's pg_class row */ + pg_class = table_open(RelationRelationId, RowExclusiveLock); + + tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(indexId)); + if (!HeapTupleIsValid(tuple)) + elog(ERROR, "cache lookup failed for relation %u", indexId); + rd_rel = (Form_pg_class) GETSTRUCT(tuple); + + /* + * Mark the relation as ready to be dropped at transaction commit, + * before making visible the new tablespace change so as this won't + * miss things. + */ + RelationDropStorage(iRel); + + /* Update the pg_class row */ + rd_rel->reltablespace = options->tablespaceOid; + CatalogTupleUpdate(pg_class, &tuple->t_self, tuple); + + heap_freetuple(tuple); + + table_close(pg_class, RowExclusiveLock); + + RelationAssumeNewRelfilenode(iRel); + + /* Make sure the reltablespace change is visible */ + CommandCounterIncrement(); + } + /* * All predicate locks on the index are about to be made invalid. Promote * them to relation locks on the heap. @@ -3814,6 +3898,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence, * reindex_relation - This routine is used to recreate all indexes * of a relation (and optionally its toast relation too, if any). * + * "tablespaceOid" is the tablespace where the relation's indexes will be + * rebuilt, or InvalidOid to keep each index on its current tablespace. + * * "flags" is a bitmask that can include any combination of these bits: * * REINDEX_REL_PROCESS_TOAST: if true, process the toast table too (if any). @@ -3964,9 +4051,14 @@ reindex_relation(Oid relid, int flags, ReindexOptions *options) /* * Note that this should fail if the toast relation is missing, so * reset REINDEXOPT_MISSING_OK. + * + * Even if a table's indexes were moved to a new tablespace, the index + * on its toast table is not normally moved. */ ReindexOptions newoptions = *options; newoptions.flags &= ~(REINDEXOPT_MISSING_OK); + newoptions.tablespaceOid = allowSystemTableMods ? + options->tablespaceOid : InvalidOid; result |= reindex_relation(toast_relid, flags, &newoptions); } diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index edd394ed4e..811ceb12b7 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2475,6 +2475,7 @@ ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel) ListCell *lc; bool concurrently = false; bool verbose = false; + char *tablespace = NULL; /* Parse option list */ foreach(lc, stmt->params) @@ -2485,6 +2486,8 @@ ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel) verbose = defGetBoolean(opt); else if (strcmp(opt->defname, "concurrently") == 0) concurrently = defGetBoolean(opt); + else if (strcmp(opt->defname, "tablespace") == 0) + tablespace = defGetString(opt); else ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -2501,6 +2504,9 @@ ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel) (verbose ? REINDEXOPT_VERBOSE : 0) | (concurrently ? REINDEXOPT_CONCURRENTLY : 0); + options.tablespaceOid = tablespace != NULL ? + get_tablespace_oid(tablespace, false) : InvalidOid; + switch (stmt->kind) { case REINDEX_OBJECT_INDEX: @@ -2728,7 +2734,10 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, MemoryContext old; List *relids = NIL; int num_keys; + bool concurrent_warning = false; + bool tablespace_warning = false; + bool mapped_warning = false; AssertArg(objectName); Assert(objectKind == REINDEX_OBJECT_SCHEMA || @@ -2855,6 +2864,35 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, continue; } + if (OidIsValid(options->tablespaceOid) && + IsSystemClass(relid, classtuple)) + { + if (!allowSystemTableMods) + { + /* Skip all system relations, if not allowSystemTableMods */ + if (!tablespace_warning) + ereport(WARNING, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("cannot change tablespace of indexes on system relations, skipping all"))); + tablespace_warning = true; + continue; + } + else if (!OidIsValid(classtuple->relfilenode)) + { + /* + * Skip all mapped relations if TABLESPACE is specified. + * OidIsValid(relfilenode) checks that, similar to + * RelationIsMapped(). + */ + if (!mapped_warning) + ereport(WARNING, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot change tablespace of indexes on mapped relations, skipping all"))); + mapped_warning = true; + continue; + } + } + /* Save the list of relation OIDs in private context */ old = MemoryContextSwitchTo(private_context); @@ -2903,6 +2941,41 @@ reindex_error_callback(void *arg) errinfo->relnamespace, errinfo->relname); } +/* + * This is mostly duplicating ATExecSetTableSpaceNoStorage, + * which should maybe be factored out to a library function. + */ +static void +set_rel_tablespace(Oid reloid, Oid tablespaceOid) +{ + Relation pg_class; + HeapTuple tuple; + Form_pg_class rd_rel; + Oid oldTablespaceOid; + + /* Get a modifiable copy of the relation's pg_class row */ + pg_class = table_open(RelationRelationId, RowExclusiveLock); + + tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid)); + if (!HeapTupleIsValid(tuple)) + elog(ERROR, "cache lookup failed for relation %u", reloid); + rd_rel = (Form_pg_class) GETSTRUCT(tuple); + + /* No work if no change in tablespace. */ + oldTablespaceOid = rd_rel->reltablespace; + if (tablespaceOid != oldTablespaceOid || + (tablespaceOid == MyDatabaseTableSpace && OidIsValid(oldTablespaceOid))) + { + /* Update the pg_class row */ + rd_rel->reltablespace = (tablespaceOid == MyDatabaseTableSpace) ? + InvalidOid : tablespaceOid; + CatalogTupleUpdate(pg_class, &tuple->t_self, tuple); + } + + heap_freetuple(tuple); + table_close(pg_class, RowExclusiveLock); +} + /* * ReindexPartitions * @@ -2968,9 +3041,27 @@ ReindexPartitions(Oid relid, ReindexOptions *options, bool isTopLevel) MemoryContext old_context; /* - * This discards partitioned tables, partitioned indexes and foreign - * tables. + * Foreign tables and partitioned relations are not themselves + * reindexed - leaf partitions are processed directly. But any + * tablespace change is recorded in the catalog for partitioned + * relations. */ + if (partkind == RELKIND_PARTITIONED_INDEX) + (void) set_rel_tablespace(partoid, options->tablespaceOid); + else if (partkind == RELKIND_PARTITIONED_TABLE) + { + Relation rel = table_open(partoid, ShareLock); + List *indexIds = RelationGetIndexList(rel); + ListCell *lc; + + table_close(rel, NoLock); + foreach (lc, indexIds) + { + Oid indexid = lfirst_oid(lc); + (void) set_rel_tablespace(indexid, options->tablespaceOid); + } + } + if (!RELKIND_HAS_STORAGE(partkind)) continue; @@ -3364,6 +3455,13 @@ ReindexRelationConcurrently(Oid relationOid, ReindexOptions *options) return false; } + /* It's not a shared catalog, so refuse to move it to shared tablespace */ + if (options->tablespaceOid == GLOBALTABLESPACE_OID) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot move non-shared relation to tablespace \"%s\"", + get_tablespace_name(options->tablespaceOid)))); + Assert(heapRelationIds != NIL); /*----- @@ -3427,6 +3525,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexOptions *options) /* Create new index definition based on given index */ newIndexId = index_concurrently_create_copy(heapRel, indexId, + options->tablespaceOid, concurrentName); /* diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 3a43c09bf6..65ebf911f3 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -3577,7 +3577,9 @@ 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("CONCURRENTLY", "VERBOSE"); + COMPLETE_WITH("CONCURRENTLY", "TABLESPACE", "VERBOSE"); + else if (TailMatches("TABLESPACE")) + COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces); } /* SECURITY LABEL */ diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h index 89394b648e..b30887e02d 100644 --- a/src/include/catalog/index.h +++ b/src/include/catalog/index.h @@ -33,6 +33,7 @@ typedef enum typedef struct ReindexOptions { bits32 flags; /* bitmask of REINDEXOPT_* */ + Oid tablespaceOid; /* tablespace to rebuild index */ } ReindexOptions; /* flag bits for ReindexOptions->flags */ @@ -92,6 +93,7 @@ extern Oid index_create(Relation heapRelation, extern Oid index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, + Oid tablespaceOid, const char *newName); extern void index_concurrently_build(Oid heapRelationId, diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source index a5f61a35dc..5d8a22cffb 100644 --- a/src/test/regress/input/tablespace.source +++ b/src/test/regress/input/tablespace.source @@ -17,6 +17,48 @@ ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter = true); -- f ALTER TABLESPACE regress_tblspace RESET (random_page_cost = 2.0); -- fail ALTER TABLESPACE regress_tblspace RESET (random_page_cost, effective_io_concurrency); -- ok +-- create table to test REINDEX with TABLESPACE change +CREATE TABLE regress_tblspace_test_tbl (num1 bigint, num2 double precision, num3 double precision); +INSERT INTO regress_tblspace_test_tbl (num1, num2, num3) + SELECT round(random()*100), random(), random()*42 + FROM generate_series(1, 20000) s(i); +CREATE INDEX regress_tblspace_test_tbl_idx ON regress_tblspace_test_tbl (num1); + +-- check that REINDEX with TABLESPACE change is transactional +BEGIN; +REINDEX (TABLESPACE regress_tblspace) INDEX regress_tblspace_test_tbl_idx; +REINDEX (TABLESPACE regress_tblspace) TABLE regress_tblspace_test_tbl; +ROLLBACK; +SELECT relname FROM pg_class +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace'); + +-- first, let us reindex and move the entire database, after that return everything back +REINDEX (TABLESPACE regress_tblspace) DATABASE regression; -- ok with warning +REINDEX (TABLESPACE pg_default) DATABASE regression; -- ok with warning +SELECT relname FROM pg_class +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace'); + +-- check REINDEX with TABLESPACE change +REINDEX (TABLESPACE regress_tblspace) INDEX regress_tblspace_test_tbl_idx; -- ok +REINDEX (TABLESPACE regress_tblspace) TABLE regress_tblspace_test_tbl; -- ok +REINDEX (TABLESPACE regress_tblspace) TABLE pg_authid; -- fail +REINDEX (TABLESPACE regress_tblspace) SYSTEM CONCURRENTLY postgres; -- fail +REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY pg_am; -- fail +REINDEX (TABLESPACE pg_global) INDEX regress_tblspace_test_tbl_idx; -- fail +REINDEX (TABLESPACE regress_tblspace) TABLE pg_am; -- fail + +-- check that all relations moved to new tablespace +SELECT relname FROM pg_class +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace') +ORDER BY relname; + +-- move indexes back to pg_default tablespace +REINDEX (TABLESPACE pg_default) TABLE CONCURRENTLY regress_tblspace_test_tbl; -- ok + +-- check that all relations moved back to pg_default +SELECT relname FROM pg_class +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace'); + -- create a schema we can use CREATE SCHEMA testschema; @@ -96,6 +138,14 @@ SELECT relname, spcname FROM pg_catalog.pg_tablespace t, pg_catalog.pg_class c \d testschema.part_a_idx \d+ testschema.part_a_idx +-- REINDEX partitioned indexes to new tablespace +REINDEX (TABLESPACE pg_default) TABLE testschema.part; +\d testschema.part +\d testschema.part1 +REINDEX (CONCURRENTLY, TABLESPACE regress_tblspace) INDEX testschema.part_a_idx; +\d testschema.part +\d testschema.part1 + -- partitioned rels cannot specify the default tablespace. These fail: CREATE TABLE testschema.dflt (a int PRIMARY KEY) PARTITION BY LIST (a) TABLESPACE pg_default; CREATE TABLE testschema.dflt (a int PRIMARY KEY USING INDEX TABLESPACE pg_default) PARTITION BY LIST (a); @@ -279,6 +329,9 @@ ALTER TABLE ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default -- Should succeed DROP TABLESPACE regress_tblspace_renamed; +DROP INDEX regress_tblspace_test_tbl_idx; +DROP TABLE regress_tblspace_test_tbl; + DROP SCHEMA testschema CASCADE; DROP ROLE regress_tablespace_user1; diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source index 162b591b31..1169f0318b 100644 --- a/src/test/regress/output/tablespace.source +++ b/src/test/regress/output/tablespace.source @@ -20,6 +20,65 @@ ERROR: unrecognized parameter "some_nonexistent_parameter" ALTER TABLESPACE regress_tblspace RESET (random_page_cost = 2.0); -- fail ERROR: RESET must not include values for parameters ALTER TABLESPACE regress_tblspace RESET (random_page_cost, effective_io_concurrency); -- ok +-- create table to test REINDEX with TABLESPACE change +CREATE TABLE regress_tblspace_test_tbl (num1 bigint, num2 double precision, num3 double precision); +INSERT INTO regress_tblspace_test_tbl (num1, num2, num3) + SELECT round(random()*100), random(), random()*42 + FROM generate_series(1, 20000) s(i); +CREATE INDEX regress_tblspace_test_tbl_idx ON regress_tblspace_test_tbl (num1); +-- check that REINDEX with TABLESPACE change is transactional +BEGIN; +REINDEX (TABLESPACE regress_tblspace) INDEX regress_tblspace_test_tbl_idx; +REINDEX (TABLESPACE regress_tblspace) TABLE regress_tblspace_test_tbl; +ROLLBACK; +SELECT relname FROM pg_class +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace'); + relname +--------- +(0 rows) + +-- first, let us reindex and move the entire database, after that return everything back +REINDEX (TABLESPACE regress_tblspace) DATABASE regression; -- ok with warning +WARNING: cannot change tablespace of indexes on system relations, skipping all +REINDEX (TABLESPACE pg_default) DATABASE regression; -- ok with warning +WARNING: cannot change tablespace of indexes on system relations, skipping all +SELECT relname FROM pg_class +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace'); + relname +--------- +(0 rows) + +-- check REINDEX with TABLESPACE change +REINDEX (TABLESPACE regress_tblspace) INDEX regress_tblspace_test_tbl_idx; -- ok +REINDEX (TABLESPACE regress_tblspace) TABLE regress_tblspace_test_tbl; -- ok +REINDEX (TABLESPACE regress_tblspace) TABLE pg_authid; -- fail +ERROR: permission denied: "pg_authid_rolname_index" is a system catalog +REINDEX (TABLESPACE regress_tblspace) SYSTEM CONCURRENTLY postgres; -- fail +ERROR: cannot reindex system catalogs concurrently +REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY pg_am; -- fail +ERROR: cannot reindex system catalogs concurrently +REINDEX (TABLESPACE pg_global) INDEX regress_tblspace_test_tbl_idx; -- fail +ERROR: cannot move non-shared relation to tablespace "pg_global" +REINDEX (TABLESPACE regress_tblspace) TABLE pg_am; -- fail +ERROR: permission denied: "pg_am_name_index" is a system catalog +-- check that all relations moved to new tablespace +SELECT relname FROM pg_class +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace') +ORDER BY relname; + relname +------------------------------- + regress_tblspace_test_tbl_idx +(1 row) + +-- move indexes back to pg_default tablespace +REINDEX (TABLESPACE pg_default) TABLE CONCURRENTLY regress_tblspace_test_tbl; -- ok +-- check that all relations moved back to pg_default +SELECT relname FROM pg_class +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace'); + relname +--------- +(0 rows) + -- create a schema we can use CREATE SCHEMA testschema; -- try a table @@ -199,6 +258,47 @@ Partitions: testschema.part1_a_idx, testschema.part2_a_idx Tablespace: "regress_tblspace" +-- REINDEX partitioned indexes to new tablespace +REINDEX (TABLESPACE pg_default) TABLE testschema.part; +\d testschema.part + Partitioned table "testschema.part" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | +Partition key: LIST (a) +Indexes: + "part_a_idx" btree (a) +Number of partitions: 2 (Use \d+ to list them.) + +\d testschema.part1 + Table "testschema.part1" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | +Partition of: testschema.part FOR VALUES IN (1) +Indexes: + "part1_a_idx" btree (a) + +REINDEX (CONCURRENTLY, TABLESPACE regress_tblspace) INDEX testschema.part_a_idx; +\d testschema.part + Partitioned table "testschema.part" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | +Partition key: LIST (a) +Indexes: + "part_a_idx" btree (a), tablespace "regress_tblspace" +Number of partitions: 2 (Use \d+ to list them.) + +\d testschema.part1 + Table "testschema.part1" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | +Partition of: testschema.part FOR VALUES IN (1) +Indexes: + "part1_a_idx" btree (a), tablespace "regress_tblspace" + -- partitioned rels cannot specify the default tablespace. These fail: CREATE TABLE testschema.dflt (a int PRIMARY KEY) PARTITION BY LIST (a) TABLESPACE pg_default; ERROR: cannot specify default tablespace for partitioned relations @@ -736,6 +836,8 @@ ALTER TABLE ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default NOTICE: no matching relations in tablespace "regress_tblspace_renamed" found -- Should succeed DROP TABLESPACE regress_tblspace_renamed; +DROP INDEX regress_tblspace_test_tbl_idx; +DROP TABLE regress_tblspace_test_tbl; DROP SCHEMA testschema CASCADE; NOTICE: drop cascades to 6 other objects DETAIL: drop cascades to table testschema.foo -- 2.17.0