From c92be47661e551f69330bfbc85407d0f49897ed0 Mon Sep 17 00:00:00 2001 From: nkey Date: Tue, 31 Dec 2024 14:24:48 +0100 Subject: [PATCH v15 10/12] Remove PROC_IN_SAFE_IC optimization Remove the optimization that allowed concurrent index builds to ignore other concurrent builds of "safe" indexes (those without expressions or predicates). This optimization is no longer needed with the new snapshot handling approach that uses periodically refreshed snapshots instead of a single reference snapshot. The change greatly simplifies the concurrent index build code by: - Removing the PROC_IN_SAFE_IC process status flag - Removing all set_indexsafe_procflags() calls and related logic - Removing special case handling in GetCurrentVirtualXIDs() - Removing related test cases and injection points This is part of improving concurrent index builds to better handle xmin propagation during long-running operations. --- src/backend/access/brin/brin.c | 6 +- src/backend/access/nbtree/nbtsort.c | 6 +- src/backend/commands/indexcmds.c | 142 +----------------- src/include/storage/proc.h | 8 +- src/test/modules/injection_points/Makefile | 2 +- .../expected/reindex_conc.out | 51 ------- src/test/modules/injection_points/meson.build | 1 - .../injection_points/sql/reindex_conc.sql | 28 ---- 8 files changed, 11 insertions(+), 233 deletions(-) delete mode 100644 src/test/modules/injection_points/expected/reindex_conc.out delete mode 100644 src/test/modules/injection_points/sql/reindex_conc.sql diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index d8317787251..0f839395c78 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -2889,11 +2889,9 @@ _brin_parallel_build_main(dsm_segment *seg, shm_toc *toc) int sortmem; /* - * The only possible status flag that can be set to the parallel worker is - * PROC_IN_SAFE_IC. + * There are no possible status flag that can be set to the parallel worker. */ - Assert((MyProc->statusFlags == 0) || - (MyProc->statusFlags == PROC_IN_SAFE_IC)); + Assert(MyProc->statusFlags == 0); /* Set debug_query_string for individual workers first */ sharedquery = shm_toc_lookup(toc, PARALLEL_KEY_QUERY_TEXT, true); diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c index 62e975016ad..1eb4299826e 100644 --- a/src/backend/access/nbtree/nbtsort.c +++ b/src/backend/access/nbtree/nbtsort.c @@ -1911,11 +1911,9 @@ _bt_parallel_build_main(dsm_segment *seg, shm_toc *toc) #endif /* BTREE_BUILD_STATS */ /* - * The only possible status flag that can be set to the parallel worker is - * PROC_IN_SAFE_IC. + * There are no possible status flag that can be set to the parallel worker. */ - Assert((MyProc->statusFlags == 0) || - (MyProc->statusFlags == PROC_IN_SAFE_IC)); + Assert(MyProc->statusFlags == 0); /* Set debug_query_string for individual workers first */ sharedquery = shm_toc_lookup(toc, PARALLEL_KEY_QUERY_TEXT, true); diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 7805e43178f..1467cd89930 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -115,7 +115,6 @@ static bool ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const ReindexParams *params); static void update_relispartition(Oid relationId, bool newval); -static inline void set_indexsafe_procflags(void); /* * callback argument type for RangeVarCallbackForReindexIndex() @@ -418,10 +417,7 @@ CompareOpclassOptions(const Datum *opts1, const Datum *opts2, int natts) * lazy VACUUMs, because they won't be fazed by missing index entries * either. (Manual ANALYZEs, however, can't be excluded because they * might be within transactions that are going to do arbitrary operations - * later.) Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY - * on indexes that are neither expressional nor partial are also safe to - * ignore, since we know that those processes won't examine any data - * outside the table they're indexing. + * later.) * * Also, GetCurrentVirtualXIDs never reports our own vxid, so we need not * check for that. @@ -442,8 +438,7 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress) VirtualTransactionId *old_snapshots; old_snapshots = GetCurrentVirtualXIDs(limitXmin, true, false, - PROC_IS_AUTOVACUUM | PROC_IN_VACUUM - | PROC_IN_SAFE_IC, + PROC_IS_AUTOVACUUM | PROC_IN_VACUUM, &n_old_snapshots); if (progress) pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots); @@ -463,8 +458,7 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress) newer_snapshots = GetCurrentVirtualXIDs(limitXmin, true, false, - PROC_IS_AUTOVACUUM | PROC_IN_VACUUM - | PROC_IN_SAFE_IC, + PROC_IS_AUTOVACUUM | PROC_IN_VACUUM, &n_newer_snapshots); for (j = i; j < n_old_snapshots; j++) { @@ -578,7 +572,6 @@ DefineIndex(Oid tableId, amoptions_function amoptions; bool exclusion; bool partitioned; - bool safe_index; Datum reloptions; int16 *coloptions; IndexInfo *indexInfo; @@ -1187,10 +1180,6 @@ DefineIndex(Oid tableId, } } - /* Is index safe for others to ignore? See set_indexsafe_procflags() */ - safe_index = indexInfo->ii_Expressions == NIL && - indexInfo->ii_Predicate == NIL; - /* * Report index creation if appropriate (delay this till after most of the * error checks) @@ -1677,10 +1666,6 @@ DefineIndex(Oid tableId, CommitTransactionCommand(); StartTransactionCommand(); - /* Tell concurrent index builds to ignore us, if index qualifies */ - if (safe_index) - set_indexsafe_procflags(); - /* * The index is now visible, so we can report the OID. While on it, * include the report for the beginning of phase 2. @@ -1735,9 +1720,6 @@ DefineIndex(Oid tableId, CommitTransactionCommand(); StartTransactionCommand(); - /* Tell concurrent index builds to ignore us, if index qualifies */ - if (safe_index) - set_indexsafe_procflags(); pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_2); /* @@ -1767,10 +1749,6 @@ DefineIndex(Oid tableId, CommitTransactionCommand(); StartTransactionCommand(); - /* Tell concurrent index builds to ignore us, if index qualifies */ - if (safe_index) - set_indexsafe_procflags(); - /* * Phase 3 of concurrent index build * @@ -1796,9 +1774,7 @@ DefineIndex(Oid tableId, CommitTransactionCommand(); StartTransactionCommand(); - /* Tell concurrent index builds to ignore us, if index qualifies */ - if (safe_index) - set_indexsafe_procflags(); + /* * Merge content of auxiliary and target indexes - insert any missing index entries. */ @@ -1815,9 +1791,6 @@ DefineIndex(Oid tableId, CommitTransactionCommand(); StartTransactionCommand(); - /* Tell concurrent index builds to ignore us, if index qualifies */ - if (safe_index) - set_indexsafe_procflags(); /* We should now definitely not be advertising any xmin. */ Assert(MyProc->xmin == InvalidTransactionId); @@ -1858,10 +1831,6 @@ DefineIndex(Oid tableId, CommitTransactionCommand(); StartTransactionCommand(); - /* Tell concurrent index builds to ignore us, if index qualifies */ - if (safe_index) - set_indexsafe_procflags(); - pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_5); /* Now wait for all transaction to see auxiliary as "non-ready for inserts" */ @@ -1882,10 +1851,6 @@ DefineIndex(Oid tableId, CommitTransactionCommand(); StartTransactionCommand(); - /* Tell concurrent index builds to ignore us, if index qualifies */ - if (safe_index) - set_indexsafe_procflags(); - pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_6); /* Now wait for all transaction to ignore auxiliary because it is dead */ @@ -3625,7 +3590,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein Oid auxIndexId; Oid tableId; Oid amId; - bool safe; /* for set_indexsafe_procflags */ } ReindexIndexInfo; List *heapRelationIds = NIL; List *indexIds = NIL; @@ -3997,17 +3961,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein save_nestlevel = NewGUCNestLevel(); RestrictSearchPath(); - /* determine safety of this index for set_indexsafe_procflags */ - idx->safe = (RelationGetIndexExpressions(indexRel) == NIL && - RelationGetIndexPredicate(indexRel) == NIL); - -#ifdef USE_INJECTION_POINTS - if (idx->safe) - INJECTION_POINT("reindex-conc-index-safe"); - else - INJECTION_POINT("reindex-conc-index-not-safe"); -#endif - idx->tableId = RelationGetRelid(heapRel); idx->amId = indexRel->rd_rel->relam; @@ -4067,7 +4020,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein newidx = palloc_object(ReindexIndexInfo); newidx->indexId = newIndexId; newidx->auxIndexId = auxIndexId; - newidx->safe = idx->safe; newidx->tableId = idx->tableId; newidx->amId = idx->amId; @@ -4160,11 +4112,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein CommitTransactionCommand(); StartTransactionCommand(); - /* - * Because we don't take a snapshot in this transaction, there's no need - * to set the PROC_IN_SAFE_IC flag here. - */ - /* * Phase 2 of REINDEX CONCURRENTLY * @@ -4195,10 +4142,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein */ CHECK_FOR_INTERRUPTS(); - /* Tell concurrent indexing to ignore us, if index qualifies */ - if (newidx->safe) - set_indexsafe_procflags(); - /* Build auxiliary index, it is fast - without any actual heap scan, just an empty index. */ index_concurrently_build(newidx->tableId, newidx->auxIndexId); @@ -4207,11 +4150,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein StartTransactionCommand(); - /* - * Because we don't take a snapshot in this transaction, there's no need - * to set the PROC_IN_SAFE_IC flag here. - */ - pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_2); /* @@ -4236,10 +4174,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein */ CHECK_FOR_INTERRUPTS(); - /* Tell concurrent indexing to ignore us, if index qualifies */ - if (newidx->safe) - set_indexsafe_procflags(); - /* * Update progress for the index to build, with the correct parent * table involved. @@ -4259,11 +4193,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein StartTransactionCommand(); - /* - * Because we don't take a snapshot or Xid in this transaction, there's no - * need to set the PROC_IN_SAFE_IC flag here. - */ - pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_3); WaitForLockersMultiple(lockTags, ShareLock, true); @@ -4284,10 +4213,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein */ CHECK_FOR_INTERRUPTS(); - /* Tell concurrent indexing to ignore us, if index qualifies */ - if (newidx->safe) - set_indexsafe_procflags(); - /* * Updating pg_index might involve TOAST table access, so ensure we * have a valid snapshot. @@ -4320,10 +4245,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein */ CHECK_FOR_INTERRUPTS(); - /* Tell concurrent indexing to ignore us, if index qualifies */ - if (newidx->safe) - set_indexsafe_procflags(); - /* * Update progress for the index to build, with the correct parent * table involved. @@ -4351,9 +4272,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein * interesting tuples. But since it might not contain tuples deleted * just before the latest snap was taken, we have to wait out any * transactions that might have older snapshots. - * - * Because we don't take a snapshot or Xid in this transaction, - * there's no need to set the PROC_IN_SAFE_IC flag here. */ pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_4); @@ -4375,13 +4293,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein INJECTION_POINT("reindex_relation_concurrently_before_swap"); StartTransactionCommand(); - /* - * Because this transaction only does catalog manipulations and doesn't do - * any index operations, we can set the PROC_IN_SAFE_IC flag here - * unconditionally. - */ - set_indexsafe_procflags(); - forboth(lc, indexIds, lc2, newIndexIds) { ReindexIndexInfo *oldidx = lfirst(lc); @@ -4437,12 +4348,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein CommitTransactionCommand(); StartTransactionCommand(); - /* - * While we could set PROC_IN_SAFE_IC if all indexes qualified, there's no - * real need for that, because we only acquire an Xid after the wait is - * done, and that lasts for a very short period. - */ - /* * Phase 5 of REINDEX CONCURRENTLY * @@ -4504,12 +4409,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein CommitTransactionCommand(); StartTransactionCommand(); - /* - * While we could set PROC_IN_SAFE_IC if all indexes qualified, there's no - * real need for that, because we only acquire an Xid after the wait is - * done, and that lasts for a very short period. - */ - /* * Phase 6 of REINDEX CONCURRENTLY * @@ -4769,36 +4668,3 @@ update_relispartition(Oid relationId, bool newval) table_close(classRel, RowExclusiveLock); } -/* - * Set the PROC_IN_SAFE_IC flag in MyProc->statusFlags. - * - * When doing concurrent index builds, we can set this flag - * to tell other processes concurrently running CREATE - * INDEX CONCURRENTLY or REINDEX CONCURRENTLY to ignore us when - * doing their waits for concurrent snapshots. On one hand it - * avoids pointlessly waiting for a process that's not interesting - * anyway; but more importantly it avoids deadlocks in some cases. - * - * This can be done safely only for indexes that don't execute any - * expressions that could access other tables, so index must not be - * expressional nor partial. Caller is responsible for only calling - * this routine when that assumption holds true. - * - * (The flag is reset automatically at transaction end, so it must be - * set for each transaction.) - */ -static inline void -set_indexsafe_procflags(void) -{ - /* - * This should only be called before installing xid or xmin in MyProc; - * otherwise, concurrent processes could see an Xmin that moves backwards. - */ - Assert(MyProc->xid == InvalidTransactionId && - MyProc->xmin == InvalidTransactionId); - - LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); - MyProc->statusFlags |= PROC_IN_SAFE_IC; - ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; - LWLockRelease(ProcArrayLock); -} diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index 20777f7d5ae..4bd24bc02d4 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -56,10 +56,6 @@ struct XidCache */ #define PROC_IS_AUTOVACUUM 0x01 /* is it an autovac worker? */ #define PROC_IN_VACUUM 0x02 /* currently running lazy vacuum */ -#define PROC_IN_SAFE_IC 0x04 /* currently running CREATE INDEX - * CONCURRENTLY or REINDEX - * CONCURRENTLY on non-expressional, - * non-partial index */ #define PROC_VACUUM_FOR_WRAPAROUND 0x08 /* set by autovac only */ #define PROC_IN_LOGICAL_DECODING 0x10 /* currently doing logical * decoding outside xact */ @@ -69,13 +65,13 @@ struct XidCache /* flags reset at EOXact */ #define PROC_VACUUM_STATE_MASK \ - (PROC_IN_VACUUM | PROC_IN_SAFE_IC | PROC_VACUUM_FOR_WRAPAROUND) + (PROC_IN_VACUUM | PROC_VACUUM_FOR_WRAPAROUND) /* * Xmin-related flags. Make sure any flags that affect how the process' Xmin * value is interpreted by VACUUM are included here. */ -#define PROC_XMIN_FLAGS (PROC_IN_VACUUM | PROC_IN_SAFE_IC) +#define PROC_XMIN_FLAGS (PROC_IN_VACUUM) /* * We allow a limited number of "weak" relation locks (AccessShareLock, diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 19d26408c2a..82acf3006bd 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -11,7 +11,7 @@ EXTENSION = injection_points DATA = injection_points--1.0.sql PGFILEDESC = "injection_points - facility for injection points" -REGRESS = injection_points hashagg reindex_conc cic_reset_snapshots +REGRESS = injection_points hashagg cic_reset_snapshots REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress ISOLATION = basic inplace syscache-update-pruned diff --git a/src/test/modules/injection_points/expected/reindex_conc.out b/src/test/modules/injection_points/expected/reindex_conc.out deleted file mode 100644 index db8de4bbe85..00000000000 --- a/src/test/modules/injection_points/expected/reindex_conc.out +++ /dev/null @@ -1,51 +0,0 @@ --- Tests for REINDEX CONCURRENTLY -CREATE EXTENSION injection_points; --- Check safety of indexes with predicates and expressions. -SELECT injection_points_set_local(); - injection_points_set_local ----------------------------- - -(1 row) - -SELECT injection_points_attach('reindex-conc-index-safe', 'notice'); - injection_points_attach -------------------------- - -(1 row) - -SELECT injection_points_attach('reindex-conc-index-not-safe', 'notice'); - injection_points_attach -------------------------- - -(1 row) - -CREATE SCHEMA reindex_inj; -CREATE TABLE reindex_inj.tbl(i int primary key, updated_at timestamp); -CREATE UNIQUE INDEX ind_simple ON reindex_inj.tbl(i); -CREATE UNIQUE INDEX ind_expr ON reindex_inj.tbl(ABS(i)); -CREATE UNIQUE INDEX ind_pred ON reindex_inj.tbl(i) WHERE mod(i, 2) = 0; -CREATE UNIQUE INDEX ind_expr_pred ON reindex_inj.tbl(abs(i)) WHERE mod(i, 2) = 0; -REINDEX INDEX CONCURRENTLY reindex_inj.ind_simple; -NOTICE: notice triggered for injection point reindex-conc-index-safe -REINDEX INDEX CONCURRENTLY reindex_inj.ind_expr; -NOTICE: notice triggered for injection point reindex-conc-index-not-safe -REINDEX INDEX CONCURRENTLY reindex_inj.ind_pred; -NOTICE: notice triggered for injection point reindex-conc-index-not-safe -REINDEX INDEX CONCURRENTLY reindex_inj.ind_expr_pred; -NOTICE: notice triggered for injection point reindex-conc-index-not-safe --- Cleanup -SELECT injection_points_detach('reindex-conc-index-safe'); - injection_points_detach -------------------------- - -(1 row) - -SELECT injection_points_detach('reindex-conc-index-not-safe'); - injection_points_detach -------------------------- - -(1 row) - -DROP TABLE reindex_inj.tbl; -DROP SCHEMA reindex_inj; -DROP EXTENSION injection_points; diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 8476bfe72a7..bddf22df3ac 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -36,7 +36,6 @@ tests += { 'sql': [ 'injection_points', 'hashagg', - 'reindex_conc', 'cic_reset_snapshots', ], 'regress_args': ['--dlpath', meson.build_root() / 'src/test/regress'], diff --git a/src/test/modules/injection_points/sql/reindex_conc.sql b/src/test/modules/injection_points/sql/reindex_conc.sql deleted file mode 100644 index 6cf211e6d5d..00000000000 --- a/src/test/modules/injection_points/sql/reindex_conc.sql +++ /dev/null @@ -1,28 +0,0 @@ --- Tests for REINDEX CONCURRENTLY -CREATE EXTENSION injection_points; - --- Check safety of indexes with predicates and expressions. -SELECT injection_points_set_local(); -SELECT injection_points_attach('reindex-conc-index-safe', 'notice'); -SELECT injection_points_attach('reindex-conc-index-not-safe', 'notice'); - -CREATE SCHEMA reindex_inj; -CREATE TABLE reindex_inj.tbl(i int primary key, updated_at timestamp); - -CREATE UNIQUE INDEX ind_simple ON reindex_inj.tbl(i); -CREATE UNIQUE INDEX ind_expr ON reindex_inj.tbl(ABS(i)); -CREATE UNIQUE INDEX ind_pred ON reindex_inj.tbl(i) WHERE mod(i, 2) = 0; -CREATE UNIQUE INDEX ind_expr_pred ON reindex_inj.tbl(abs(i)) WHERE mod(i, 2) = 0; - -REINDEX INDEX CONCURRENTLY reindex_inj.ind_simple; -REINDEX INDEX CONCURRENTLY reindex_inj.ind_expr; -REINDEX INDEX CONCURRENTLY reindex_inj.ind_pred; -REINDEX INDEX CONCURRENTLY reindex_inj.ind_expr_pred; - --- Cleanup -SELECT injection_points_detach('reindex-conc-index-safe'); -SELECT injection_points_detach('reindex-conc-index-not-safe'); -DROP TABLE reindex_inj.tbl; -DROP SCHEMA reindex_inj; - -DROP EXTENSION injection_points; -- 2.43.0