From 770d96cf4c797ece2ada56969d0d46c15563f763 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Wed, 1 Apr 2020 19:00:25 -0500 Subject: [PATCH v17 7/8] pass idxtablespaceNAME to reduce error duplication Only reindex_index needs to look up the Oid or check for errors. XXX: for cluster/vacuum, it might be more friendly to check before clustering the table, rather than after clustering and re-indexing. --- src/backend/catalog/index.c | 17 +++++++------- src/backend/commands/cluster.c | 25 +++++++-------------- src/backend/commands/indexcmds.c | 35 +++++++++-------------------- src/backend/commands/tablecmds.c | 2 +- src/backend/commands/vacuum.c | 15 ++++++------- src/backend/postmaster/autovacuum.c | 2 +- src/include/catalog/index.h | 4 ++-- src/include/commands/cluster.h | 4 ++-- src/include/commands/vacuum.h | 4 ++-- 9 files changed, 41 insertions(+), 67 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index d7db92219d..9e30e305bc 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -3439,17 +3439,17 @@ IndexGetRelation(Oid indexId, bool missing_ok) * See comments of reindex_relation() for details about "tablespaceOid". */ void -reindex_index(Oid indexId, Oid tablespaceOid, bool skip_constraint_checks, +reindex_index(Oid indexId, const char *tablespace, bool skip_constraint_checks, char persistence, int options) { Relation iRel, heapRelation; Oid heapId; + Oid tablespaceOid = tablespace ? get_tablespace_oid(tablespace, false) : InvalidOid; IndexInfo *indexInfo; volatile bool skipped_constraint = false; PGRUsage ru0; bool progress = (options & REINDEXOPT_REPORT_PROGRESS) != 0; - bool set_tablespace = OidIsValid(tablespaceOid); pg_rusage_init(&ru0); @@ -3492,7 +3492,7 @@ reindex_index(Oid indexId, Oid tablespaceOid, bool skip_constraint_checks, * We don't support moving system relations into different tablespaces, * unless allow_system_table_mods=1. */ - if (set_tablespace && + if (tablespace != NULL && !allowSystemTableMods && IsSystemRelation(iRel)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), @@ -3503,7 +3503,7 @@ reindex_index(Oid indexId, Oid tablespaceOid, bool skip_constraint_checks, * We cannot support moving mapped relations into different tablespaces. * (In particular this eliminates all shared catalogs.) */ - if (set_tablespace && RelationIsMapped(iRel)) + if (tablespace != NULL && RelationIsMapped(iRel)) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot change tablespace of mapped relation \"%s\"", @@ -3516,7 +3516,6 @@ reindex_index(Oid indexId, Oid tablespaceOid, bool skip_constraint_checks, errmsg("cannot move non-shared relation to tablespace \"%s\"", get_tablespace_name(tablespaceOid)))); - /* * Don't allow reindex on temp tables of other backends ... their local * buffer manager is not going to cope. @@ -3550,7 +3549,7 @@ reindex_index(Oid indexId, Oid tablespaceOid, bool skip_constraint_checks, * Set the new tablespace for the relation. Do that only in the * case where the reindex caller wishes to enforce a new tablespace. */ - if (set_tablespace && + if (tablespace != NULL && tablespaceOid != iRel->rd_rel->reltablespace) { Relation pg_class; @@ -3758,7 +3757,7 @@ reindex_index(Oid indexId, Oid tablespaceOid, bool skip_constraint_checks, * index rebuild. */ bool -reindex_relation(Oid relid, Oid tablespaceOid, int flags, int options) +reindex_relation(Oid relid, const char *tablespace, int flags, int options) { Relation rel; Oid toast_relid; @@ -3849,7 +3848,7 @@ reindex_relation(Oid relid, Oid tablespaceOid, int flags, int options) continue; } - reindex_index(indexOid, tablespaceOid, + reindex_index(indexOid, tablespace, !(flags & REINDEX_REL_CHECK_CONSTRAINTS), persistence, options); @@ -3883,7 +3882,7 @@ reindex_relation(Oid relid, Oid tablespaceOid, int flags, int options) * still hold the lock on the master table. */ if ((flags & REINDEX_REL_PROCESS_TOAST) && OidIsValid(toast_relid)) - result |= reindex_relation(toast_relid, tablespaceOid, flags, options); + result |= reindex_relation(toast_relid, tablespace, flags, options); return result; } diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 037b51e520..8601d36072 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -70,7 +70,7 @@ typedef struct } RelToCluster; -static void rebuild_relation(Relation OldHeap, Oid indexOid, Oid NewTableSpaceOid, Oid NewIdxTableSpaceOid, bool verbose); +static void rebuild_relation(Relation OldHeap, Oid indexOid, Oid NewTableSpaceOid, const char *NewIdxTableSpace, bool verbose); static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, bool verbose, bool *pSwapToastByContent, TransactionId *pFreezeXid, MultiXactId *pCutoffMulti); @@ -109,7 +109,6 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) char *tablespaceName = NULL, *idxtablespaceName = NULL; Oid tablespaceOid; - Oid idxtablespaceOid; /* Parse list of generic parameter not handled by the parser */ foreach(lc, stmt->params) @@ -134,8 +133,6 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) /* Get tablespaces to use. */ tablespaceOid = tablespaceName ? get_tablespace_oid(tablespaceName, false) : InvalidOid; - idxtablespaceOid = idxtablespaceName ? - get_tablespace_oid(idxtablespaceName, false) : InvalidOid; if (stmt->relation != NULL) { @@ -218,7 +215,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) table_close(rel, NoLock); /* Do the job. */ - cluster_rel(tableOid, indexOid, tablespaceOid, idxtablespaceOid, stmt->options); + cluster_rel(tableOid, indexOid, tablespaceOid, idxtablespaceName, stmt->options); } else { @@ -267,7 +264,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) PushActiveSnapshot(GetTransactionSnapshot()); /* Do the job. */ cluster_rel(rvtc->tableOid, rvtc->indexOid, tablespaceOid, - idxtablespaceOid, stmt->options | CLUOPT_RECHECK); + idxtablespaceName, stmt->options | CLUOPT_RECHECK); PopActiveSnapshot(); CommitTransactionCommand(); } @@ -301,7 +298,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) * InvalidOid, use the tablespace in-use instead. */ void -cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid, Oid idxtablespaceOid, int options) +cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid, const char *idxtablespace, int options) { Relation OldHeap; bool verbose = ((options & CLUOPT_VERBOSE) != 0); @@ -481,7 +478,7 @@ cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid, Oid idxtablespaceOid, TransferPredicateLocksToHeapRelation(OldHeap); /* rebuild_relation does all the dirty work */ - rebuild_relation(OldHeap, indexOid, tablespaceOid, idxtablespaceOid, verbose); + rebuild_relation(OldHeap, indexOid, tablespaceOid, idxtablespace, verbose); /* NB: rebuild_relation does table_close() on OldHeap */ @@ -640,11 +637,10 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal) * NB: this routine closes OldHeap at the right time; caller should not. */ static void -rebuild_relation(Relation OldHeap, Oid indexOid, Oid NewTablespaceOid, Oid NewIdxTablespaceOid, bool verbose) +rebuild_relation(Relation OldHeap, Oid indexOid, Oid NewTablespaceOid, const char *NewIdxTablespace, bool verbose) { Oid tableOid = RelationGetRelid(OldHeap); Oid tableSpace = OldHeap->rd_rel->reltablespace; - Oid idxtableSpace; Oid OIDNewHeap; char relpersistence; bool is_system_catalog; @@ -664,11 +660,6 @@ rebuild_relation(Relation OldHeap, Oid indexOid, Oid NewTablespaceOid, Oid NewId get_tablespace_name(tableSpace)))); } - if (OidIsValid(NewIdxTablespaceOid)) - idxtableSpace = NewIdxTablespaceOid; - else - idxtableSpace = get_rel_tablespace(indexOid); - /* Mark the correct index as clustered */ if (OidIsValid(indexOid)) mark_index_clustered(OldHeap, indexOid, true); @@ -696,7 +687,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, Oid NewTablespaceOid, Oid NewId finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog, swap_toast_by_content, false, true, frozenXid, cutoffMulti, - relpersistence, idxtableSpace); + relpersistence, NewIdxTablespace); } @@ -1425,7 +1416,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, bool is_internal, TransactionId frozenXid, MultiXactId cutoffMulti, - char newrelpersistence, Oid idxtableSpace) + char newrelpersistence, const char *idxtableSpace) { ObjectAddress object; Oid mapped_tables[4]; diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 7e4e54902e..3b07129f30 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -87,7 +87,7 @@ static char *ChooseIndexNameAddition(List *colnames); static List *ChooseIndexColumnNames(List *indexElems); static void RangeVarCallbackForReindexIndex(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); -static bool ReindexRelationConcurrently(Oid relationOid, Oid tablespaceOid, int options); +static bool ReindexRelationConcurrently(Oid relationOid, const char *tablespace, int options); static void ReindexPartitionedIndex(Relation parentIdx); static void update_relispartition(Oid relationId, bool newval); static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts); @@ -2423,7 +2423,6 @@ ReindexIndex(ReindexStmt *stmt) RangeVar *indexRelation = stmt->relation; struct ReindexIndexCallbackState state; Oid indOid; - Oid tablespaceOid = InvalidOid; Relation irel; char persistence; @@ -2459,16 +2458,12 @@ ReindexIndex(ReindexStmt *stmt) persistence = irel->rd_rel->relpersistence; - /* Define new tablespaceOid if requested */ - if (stmt->tablespacename) - tablespaceOid = get_tablespace_oid(stmt->tablespacename, false); - index_close(irel, NoLock); if (stmt->concurrent && persistence != RELPERSISTENCE_TEMP) - ReindexRelationConcurrently(indOid, tablespaceOid, stmt->options); + ReindexRelationConcurrently(indOid, stmt->tablespacename, stmt->options); else - reindex_index(indOid, tablespaceOid, false, persistence, + reindex_index(indOid, stmt->tablespacename, false, persistence, stmt->options | REINDEXOPT_REPORT_PROGRESS); } @@ -2552,7 +2547,6 @@ ReindexTable(ReindexStmt *stmt) RangeVar *relation = stmt->relation; Oid heapOid; bool result; - Oid tablespaceOid = InvalidOid; /* * The lock level used here should match reindex_relation(). @@ -2567,13 +2561,9 @@ ReindexTable(ReindexStmt *stmt) 0, RangeVarCallbackOwnsTable, NULL); - /* Define new tablespaceOid if requested */ - if (stmt->tablespacename) - tablespaceOid = get_tablespace_oid(stmt->tablespacename, false); - if (stmt->concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP) { - result = ReindexRelationConcurrently(heapOid, tablespaceOid, stmt->options); + result = ReindexRelationConcurrently(heapOid, stmt->tablespacename, stmt->options); if (!result) ereport(NOTICE, @@ -2583,7 +2573,7 @@ ReindexTable(ReindexStmt *stmt) else { result = reindex_relation(heapOid, - tablespaceOid, + stmt->tablespacename, REINDEX_REL_PROCESS_TOAST | REINDEX_REL_CHECK_CONSTRAINTS, stmt->options | REINDEXOPT_REPORT_PROGRESS); @@ -2610,7 +2600,6 @@ ReindexMultipleTables(ReindexStmt *stmt) const char *objectName = stmt->name; ReindexObjectType objectKind = stmt->kind; Oid objectOid; - Oid tablespaceOid = InvalidOid; Relation relationRelation; TableScanDesc scan; ScanKeyData scan_keys[1]; @@ -2660,10 +2649,6 @@ ReindexMultipleTables(ReindexStmt *stmt) objectName); } - /* Define new tablespaceOid if requested */ - if (stmt->tablespacename) - tablespaceOid = get_tablespace_oid(stmt->tablespacename, false); - /* * Create a memory context that will survive forced transaction commits we * do below. Since it is a child of PortalContext, it will go away @@ -2754,7 +2739,7 @@ ReindexMultipleTables(ReindexStmt *stmt) continue; } - if (OidIsValid(tablespaceOid) && + if (stmt->tablespacename && // OidIsValid(tablespaceOid) && IsSystemClass(relid, classtuple)) { if (!allowSystemTableMods) @@ -2816,7 +2801,7 @@ ReindexMultipleTables(ReindexStmt *stmt) if (stmt->concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP) { - (void) ReindexRelationConcurrently(relid, tablespaceOid, stmt->options); + (void) ReindexRelationConcurrently(relid, stmt->tablespacename, stmt->options); /* ReindexRelationConcurrently() does the verbose output */ } else @@ -2824,7 +2809,7 @@ ReindexMultipleTables(ReindexStmt *stmt) bool result; result = reindex_relation(relid, - tablespaceOid, + stmt->tablespacename, REINDEX_REL_PROCESS_TOAST | REINDEX_REL_CHECK_CONSTRAINTS, stmt->options | REINDEXOPT_REPORT_PROGRESS); @@ -2874,7 +2859,7 @@ ReindexMultipleTables(ReindexStmt *stmt) * anyway, and a non-concurrent reindex is more efficient. */ static bool -ReindexRelationConcurrently(Oid relationOid, Oid tablespaceOid, int options) +ReindexRelationConcurrently(Oid relationOid, const char *tablespace, int options) { List *heapRelationIds = NIL; List *indexIds = NIL; @@ -3148,7 +3133,7 @@ ReindexRelationConcurrently(Oid relationOid, Oid tablespaceOid, int options) /* Create new index definition based on given index */ newIndexId = index_concurrently_create_copy(heapRel, indexId, - tablespaceOid, + tablespace ? get_tablespace_oid(tablespace, false) : InvalidOid, concurrentName); /* diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 642f515655..c1911af3f2 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -1871,7 +1871,7 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged, /* * Reconstruct the indexes to match, and we're done. */ - reindex_relation(heap_relid, InvalidOid, REINDEX_REL_PROCESS_TOAST, 0); + reindex_relation(heap_relid, NULL, REINDEX_REL_PROCESS_TOAST, 0); } pgstat_count_truncate(rel); diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index b79aa3d30f..c8a307b1a9 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -111,8 +111,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) ListCell *lc; /* Tablespace to use for relations after VACUUM FULL. */ - char *tablespacename = NULL, - *idxtablespacename = NULL; + char *tablespacename = NULL; /* Set default value */ params.index_cleanup = VACOPT_TERNARY_DEFAULT; @@ -121,6 +120,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) /* By default parallel vacuum is enabled */ params.nworkers = 0; + params.idxtablespace = NULL; + /* Parse options list */ foreach(lc, vacstmt->options) { @@ -153,7 +154,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) else if (strcmp(opt->defname, "tablespace") == 0) tablespacename = defGetString(opt); else if (strcmp(opt->defname, "index_tablespace") == 0) - idxtablespacename = defGetString(opt); + params.idxtablespace = defGetString(opt); else if (strcmp(opt->defname, "parallel") == 0) { parallel_option = true; @@ -216,17 +217,15 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) errmsg("cannot specify both FULL and PARALLEL options"))); if ((params.options & VACOPT_FULL) == 0 && - (tablespacename || idxtablespacename)) + (tablespacename || params.idxtablespace)) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("incompatible TABLESPACE option"), errdetail("You can only use TABLESPACE with VACUUM FULL."))); - /* Get tablespace Oids to use. */ + /* Get tablespaces to use. */ params.tablespace_oid = tablespacename ? get_tablespace_oid(tablespacename, false) : InvalidOid; - params.idxtablespace_oid = idxtablespacename ? - get_tablespace_oid(idxtablespacename, false) : InvalidOid; /* * Make sure VACOPT_ANALYZE is specified if any column lists are present. @@ -1917,7 +1916,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params) /* VACUUM FULL is now a variant of CLUSTER; see cluster.c */ cluster_rel(relid, InvalidOid, params->tablespace_oid, - params->idxtablespace_oid, cluster_options); + params->idxtablespace, cluster_options); } else table_relation_vacuum(onerel, params, vac_strategy); diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 7de5797f9c..8f191c9ead 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -2898,7 +2898,7 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, tab->at_params.is_wraparound = wraparound; tab->at_params.log_min_duration = log_min_duration; tab->at_params.tablespace_oid = InvalidOid; - tab->at_params.idxtablespace_oid = InvalidOid; + tab->at_params.idxtablespace = NULL; tab->at_vacuum_cost_limit = vac_cost_limit; tab->at_vacuum_cost_delay = vac_cost_delay; tab->at_relname = NULL; diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h index f38e978b45..35301cf47b 100644 --- a/src/include/catalog/index.h +++ b/src/include/catalog/index.h @@ -132,7 +132,7 @@ extern void validate_index(Oid heapId, Oid indexId, Snapshot snapshot); extern void index_set_state_flags(Oid indexId, IndexStateFlagsAction action); -extern void reindex_index(Oid indexId, Oid tablespaceOid, bool skip_constraint_checks, +extern void reindex_index(Oid indexId, const char *tablespace, bool skip_constraint_checks, char relpersistence, int options); /* Flag bits for reindex_relation(): */ @@ -142,7 +142,7 @@ extern void reindex_index(Oid indexId, Oid tablespaceOid, bool skip_constraint_c #define REINDEX_REL_FORCE_INDEXES_UNLOGGED 0x08 #define REINDEX_REL_FORCE_INDEXES_PERMANENT 0x10 -extern bool reindex_relation(Oid relid, Oid tablespaceOid, int flags, int options); +extern bool reindex_relation(Oid relid, const char *tablespace, int flags, int 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 515e810505..80f9120138 100644 --- a/src/include/commands/cluster.h +++ b/src/include/commands/cluster.h @@ -20,7 +20,7 @@ extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel); -extern void cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid, Oid indextablespaceOid, int options); +extern void cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid, const char *indextablespace, int 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); @@ -35,6 +35,6 @@ extern void finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, TransactionId frozenXid, MultiXactId minMulti, char newrelpersistence, - Oid idxtablespaceOid); + const char *idxtablespace); #endif /* CLUSTER_H */ diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index d6f8e5de23..edbb123681 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -230,9 +230,9 @@ typedef struct VacuumParams */ int nworkers; - /* tablespace Oids to use for relations rebuilt by VACUUM FULL */ + /* tablespaces to use for relations rebuilt by VACUUM FULL */ Oid tablespace_oid; - Oid idxtablespace_oid; + char *idxtablespace; } VacuumParams; /* GUC parameters */ -- 2.17.0