From 43596b70d9e4ba0ec0e6b93a0bd8e888deefaf1a Mon Sep 17 00:00:00 2001 From: Mikhail Nikalayeu Date: Tue, 31 Dec 2024 14:24:48 +0100 Subject: [PATCH v25 12/12] Remove PROC_IN_SAFE_IC optimization This optimization allowed concurrent index builds to ignore other indexes without expressions or predicates. With the new snapshot handling approach that periodically refreshes snapshots, this optimization is no longer necessary. The change simplifies concurrent index build code by: - removing the PROC_IN_SAFE_IC process status flag - eliminating set_indexsafe_procflags() calls and related logic - removing special case handling in GetCurrentVirtualXIDs() - removing related test cases and injection points --- src/backend/access/brin/brin.c | 6 +- src/backend/access/gin/gininsert.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 ---- 9 files changed, 13 insertions(+), 237 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 5554cfa6f4d..cebcb777ef3 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -2893,11 +2893,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/gin/gininsert.c b/src/backend/access/gin/gininsert.c index bf26106aa5e..829ecb4ed41 100644 --- a/src/backend/access/gin/gininsert.c +++ b/src/backend/access/gin/gininsert.c @@ -2106,11 +2106,9 @@ _gin_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 4f936a6cd98..f4ea4cce04d 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 95d9ba57324..2480c6e8cf0 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -114,7 +114,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() @@ -417,10 +416,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. @@ -441,8 +437,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); @@ -462,8 +457,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++) { @@ -577,7 +571,6 @@ DefineIndex(Oid tableId, amoptions_function amoptions; bool exclusion; bool partitioned; - bool safe_index; Datum reloptions; int16 *coloptions; IndexInfo *indexInfo; @@ -1181,10 +1174,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) @@ -1670,10 +1659,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. @@ -1728,9 +1713,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); /* @@ -1760,10 +1742,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 * @@ -1789,9 +1767,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. */ @@ -1808,9 +1784,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); @@ -1851,10 +1824,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" */ @@ -1875,10 +1844,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 */ @@ -3653,7 +3618,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein Oid junkAuxIndexId; Oid tableId; Oid amId; - bool safe; /* for set_indexsafe_procflags */ } ReindexIndexInfo; List *heapRelationIds = NIL; List *indexIds = NIL; @@ -4027,17 +3991,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", NULL); - else - INJECTION_POINT("reindex-conc-index-not-safe", NULL); -#endif - idx->tableId = RelationGetRelid(heapRel); idx->amId = indexRel->rd_rel->relam; @@ -4103,7 +4056,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein newidx->indexId = newIndexId; newidx->auxIndexId = auxIndexId; newidx->junkAuxIndexId = junkAuxIndexId; - newidx->safe = idx->safe; newidx->tableId = idx->tableId; newidx->amId = idx->amId; @@ -4204,11 +4156,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 * @@ -4240,10 +4187,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); @@ -4252,11 +4195,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); /* @@ -4281,10 +4219,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. @@ -4304,11 +4238,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); @@ -4330,10 +4259,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. @@ -4369,10 +4294,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. @@ -4400,9 +4321,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); @@ -4424,13 +4342,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein INJECTION_POINT("reindex_relation_concurrently_before_swap", NULL); 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); @@ -4486,12 +4397,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 * @@ -4555,12 +4460,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 * @@ -4828,36 +4727,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 c6f5ebceefd..f47d268d6c7 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 f4a62ed1ca7..b217b1aa951 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 vacuum cic_reset_snapshots +REGRESS = injection_points hashagg vacuum 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 ba7bc0cc384..7feaf05129c 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', 'vacuum', 'cic_reset_snapshots', ], 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.48.1