Skip to content

Commit e7418f8

Browse files
anarazelanarazel
committed
Fix potential assertion failure when reindexing a pg_class index.
When reindexing individual indexes on pg_class it was possible to either trigger an assertion failure: TRAP: FailedAssertion("!(!ReindexIsProcessingIndex(((index)->rd_id))) That's because reindex_index() called SetReindexProcessing() - which enables an asserts ensuring no index insertions happen into the index - before calling RelationSetNewRelfilenode(). That not correct for indexes on pg_class, because RelationSetNewRelfilenode() updates the relevant pg_class row, which needs to update the indexes. The are two reasons this wasn't noticed earlier. Firstly the bug doesn't trigger when reindexing all of pg_class, as reindex_relation has code "hiding" all yet-to-be-reindexed indexes. Secondly, the bug only triggers when the the update to pg_class doesn't turn out to be a HOT update - otherwise there's no index insertion to trigger the bug. Most of the time there's enough space, making this bug hard to trigger. To fix, move RelationSetNewRelfilenode() to before the SetReindexProcessing() (and, together with some other code, to outside of the PG_TRY()). To make sure the error checking intended by SetReindexProcessing() is more robust, modify CatalogIndexInsert() to check ReindexIsProcessingIndex() even when the update is a HOT update. Also add a few regression tests for REINDEXing of system catalogs. The last two improvements would have prevented some of the issues fixed in 5c15606 from being introduced in the first place. Reported-By: Michael Paquier Diagnosed-By: Tom Lane and Andres Freund Author: Andres Freund Reviewed-By: Tom Lane Discussion: https://postgr.es/m/20190418011430.GA19133@paquier.xyz Backpatch: 9.4-, the bug is present in all branches
1 parent 17947af commit e7418f8

File tree

6 files changed

+91
-20
lines changed

6 files changed

+91
-20
lines changed

‎contrib/test_decoding/expected/rewrite.out

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,10 @@ COMMIT;
103103
-- repeated rewrites in different transactions
104104
VACUUM FULL pg_class;
105105
VACUUM FULL pg_class;
106+
-- reindexing of important relations / indexes
107+
REINDEX TABLE pg_class;
108+
REINDEX INDEX pg_class_oid_index;
109+
REINDEX INDEX pg_class_tblspc_relfilenode_index;
106110
INSERT INTO replication_example(somedata, testcolumn1) VALUES (5, 3);
107111
BEGIN;
108112
INSERT INTO replication_example(somedata, testcolumn1) VALUES (6, 4);

‎contrib/test_decoding/sql/rewrite.sql

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,11 @@ COMMIT;
7474
VACUUM FULL pg_class;
7575
VACUUM FULL pg_class;
7676

77+
-- reindexing of important relations / indexes
78+
REINDEX TABLE pg_class;
79+
REINDEX INDEX pg_class_oid_index;
80+
REINDEX INDEX pg_class_tblspc_relfilenode_index;
81+
7782
INSERT INTO replication_example(somedata, testcolumn1) VALUES (5, 3);
7883

7984
BEGIN;

‎src/backend/catalog/index.c

Lines changed: 24 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3143,29 +3143,35 @@ reindex_index(Oid indexId, bool skip_constraint_checks)
31433143
*/
31443144
TransferPredicateLocksToHeapRelation(iRel);
31453145

3146+
/* Fetch info needed for index_build */
3147+
indexInfo = BuildIndexInfo(iRel);
3148+
3149+
/* If requested, skip checking uniqueness/exclusion constraints */
3150+
if (skip_constraint_checks)
3151+
{
3152+
if (indexInfo->ii_Unique || indexInfo->ii_ExclusionOps != NULL)
3153+
skipped_constraint = true;
3154+
indexInfo->ii_Unique = false;
3155+
indexInfo->ii_ExclusionOps = NULL;
3156+
indexInfo->ii_ExclusionProcs = NULL;
3157+
indexInfo->ii_ExclusionStrats = NULL;
3158+
}
3159+
3160+
/*
3161+
* Build a new physical relation for the index. Need to do that before
3162+
* "officially" starting the reindexing with SetReindexProcessing -
3163+
* otherwise the necessary pg_class changes cannot be made with
3164+
* encountering assertions.
3165+
*/
3166+
RelationSetNewRelfilenode(iRel, InvalidTransactionId,
3167+
InvalidMultiXactId);
3168+
3169+
/* ensure SetReindexProcessing state isn't leaked */
31463170
PG_TRY();
31473171
{
31483172
/* Suppress use of the target index while rebuilding it */
31493173
SetReindexProcessing(heapId, indexId);
31503174

3151-
/* Fetch info needed for index_build */
3152-
indexInfo = BuildIndexInfo(iRel);
3153-
3154-
/* If requested, skip checking uniqueness/exclusion constraints */
3155-
if (skip_constraint_checks)
3156-
{
3157-
if (indexInfo->ii_Unique || indexInfo->ii_ExclusionOps != NULL)
3158-
skipped_constraint = true;
3159-
indexInfo->ii_Unique = false;
3160-
indexInfo->ii_ExclusionOps = NULL;
3161-
indexInfo->ii_ExclusionProcs = NULL;
3162-
indexInfo->ii_ExclusionStrats = NULL;
3163-
}
3164-
3165-
/* We'll build a new physical relation for the index */
3166-
RelationSetNewRelfilenode(iRel, InvalidTransactionId,
3167-
InvalidMultiXactId);
3168-
31693175
/* Initialize the index and rebuild */
31703176
/* Note: we do not need to re-establish pkey setting */
31713177
index_build(heapRelation, iRel, indexInfo, false, true);

‎src/backend/catalog/indexing.c

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,15 @@ CatalogIndexInsert(CatalogIndexState indstate, HeapTuple heapTuple)
8080
Datum values[INDEX_MAX_KEYS];
8181
bool isnull[INDEX_MAX_KEYS];
8282

83-
/* HOT update does not require index inserts */
83+
/*
84+
* HOT update does not require index inserts. But with asserts enabled we
85+
* want to check that it'd be legal to currently insert into the
86+
* table/index.
87+
*/
88+
#ifndef USE_ASSERT_CHECKING
8489
if (HeapTupleIsHeapOnly(heapTuple))
8590
return;
91+
#endif
8692

8793
/*
8894
* Get information from the state structure. Fall out if nothing to do.
@@ -104,8 +110,10 @@ CatalogIndexInsert(CatalogIndexState indstate, HeapTuple heapTuple)
104110
for (i = 0; i < numIndexes; i++)
105111
{
106112
IndexInfo *indexInfo;
113+
Relation index;
107114

108115
indexInfo = indexInfoArray[i];
116+
index = relationDescs[i];
109117

110118
/* If the index is marked as read-only, ignore it */
111119
if (!indexInfo->ii_ReadyForInserts)
@@ -118,7 +126,16 @@ CatalogIndexInsert(CatalogIndexState indstate, HeapTuple heapTuple)
118126
Assert(indexInfo->ii_Expressions == NIL);
119127
Assert(indexInfo->ii_Predicate == NIL);
120128
Assert(indexInfo->ii_ExclusionOps == NULL);
121-
Assert(relationDescs[i]->rd_index->indimmediate);
129+
Assert(index->rd_index->indimmediate);
130+
131+
/* see earlier check above */
132+
#ifdef USE_ASSERT_CHECKING
133+
if (HeapTupleIsHeapOnly(heapTuple))
134+
{
135+
Assert(!ReindexIsProcessingIndex(RelationGetRelid(index)));
136+
continue;
137+
}
138+
#endif /* USE_ASSERT_CHECKING */
122139

123140
/*
124141
* FormIndexDatum fills in its values and isnull parameters with the

‎src/test/regress/expected/create_index.out

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2804,3 +2804,21 @@ explain (costs off)
28042804
Index Cond: ((thousand = 1) AND (tenthous = 1001))
28052805
(2 rows)
28062806

2807+
--
2808+
-- check that system tables can be reindexed
2809+
--
2810+
-- whole tables
2811+
REINDEX TABLE pg_class; -- mapped, non-shared, critical
2812+
REINDEX TABLE pg_index; -- non-mapped, non-shared, critical
2813+
REINDEX TABLE pg_operator; -- non-mapped, non-shared, critical
2814+
REINDEX TABLE pg_database; -- mapped, shared, critical
2815+
REINDEX TABLE pg_shdescription; -- mapped, shared non-critical
2816+
-- Check that individual system indexes can be reindexed. That's a bit
2817+
-- different from the entire-table case because reindex_relation
2818+
-- treats e.g. pg_class special.
2819+
REINDEX INDEX pg_class_oid_index; -- mapped, non-shared, critical
2820+
REINDEX INDEX pg_class_relname_nsp_index; -- mapped, non-shared, non-critical
2821+
REINDEX INDEX pg_index_indexrelid_index; -- non-mapped, non-shared, critical
2822+
REINDEX INDEX pg_index_indrelid_index; -- non-mapped, non-shared, non-critical
2823+
REINDEX INDEX pg_database_oid_index; -- mapped, shared, critical
2824+
REINDEX INDEX pg_shdescription_o_c_index; -- mapped, shared, non-critical

‎src/test/regress/sql/create_index.sql

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -951,3 +951,24 @@ RESET enable_indexonlyscan;
951951

952952
explain (costs off)
953953
select * from tenk1 where (thousand, tenthous) in ((1,1001), (null,null));
954+
955+
--
956+
-- check that system tables can be reindexed
957+
--
958+
959+
-- whole tables
960+
REINDEX TABLE pg_class; -- mapped, non-shared, critical
961+
REINDEX TABLE pg_index; -- non-mapped, non-shared, critical
962+
REINDEX TABLE pg_operator; -- non-mapped, non-shared, critical
963+
REINDEX TABLE pg_database; -- mapped, shared, critical
964+
REINDEX TABLE pg_shdescription; -- mapped, shared non-critical
965+
966+
-- Check that individual system indexes can be reindexed. That's a bit
967+
-- different from the entire-table case because reindex_relation
968+
-- treats e.g. pg_class special.
969+
REINDEX INDEX pg_class_oid_index; -- mapped, non-shared, critical
970+
REINDEX INDEX pg_class_relname_nsp_index; -- mapped, non-shared, non-critical
971+
REINDEX INDEX pg_index_indexrelid_index; -- non-mapped, non-shared, critical
972+
REINDEX INDEX pg_index_indrelid_index; -- non-mapped, non-shared, non-critical
973+
REINDEX INDEX pg_database_oid_index; -- mapped, shared, critical
974+
REINDEX INDEX pg_shdescription_o_c_index; -- mapped, shared, non-critical

0 commit comments

Comments
 (0)