Skip to content

Commit 1da5bef

Browse files
tglsfdctglsfdc
committed
Fix assorted bugs in CREATE INDEX CONCURRENTLY.
This patch changes CREATE INDEX CONCURRENTLY so that the pg_index flag changes it makes without exclusive lock on the index are made via heap_inplace_update() rather than a normal transactional update. The latter is not very safe because moving the pg_index tuple could result in concurrent SnapshotNow scans finding it twice or not at all, thus possibly resulting in index corruption. In addition, fix various places in the code that ought to check to make sure that the indexes they are manipulating are valid and/or ready as appropriate. These represent bugs that have existed since 8.2, since a failed CREATE INDEX CONCURRENTLY could leave a corrupt or invalid index behind, and we ought not try to do anything that might fail with such an index. Also fix RelationReloadIndexInfo to ensure it copies all the pg_index columns that are allowed to change after initial creation. Previously we could have been left with stale values of some fields in an index relcache entry. It's not clear whether this actually had any user-visible consequences, but it's at least a bug waiting to happen. This is a subset of a patch already applied in 9.2 and HEAD. Back-patch into all earlier supported branches. Tom Lane and Andres Freund
1 parent 381c3b8 commit 1da5bef

File tree

12 files changed

+216
-85
lines changed

12 files changed

+216
-85
lines changed

‎src/backend/access/heap/README.HOT

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,12 @@ from the index, as well as ensuring that no one can see any inconsistent
386386
rows in a broken HOT chain (the first condition is stronger than the
387387
second). Finally, we can mark the index valid for searches.
388388

389+
Note that we do not need to set pg_index.indcheckxmin in this code path,
390+
because we have outwaited any transactions that would need to avoid using
391+
the index. (indcheckxmin is only needed because non-concurrent CREATE
392+
INDEX doesn't want to wait; its stronger lock would create too much risk of
393+
deadlock if it did.)
394+
389395

390396
Limitations and Restrictions
391397
----------------------------

‎src/backend/catalog/index.c

Lines changed: 124 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,10 @@ static void ResetReindexPending(void);
129129
* See whether an existing relation has a primary key.
130130
*
131131
* Caller must have suitable lock on the relation.
132+
*
133+
* Note: we intentionally do not check IndexIsValid here; that's because this
134+
* is used to enforce the rule that there can be only one indisprimary index,
135+
* and we want that to be true even if said index is invalid.
132136
*/
133137
static bool
134138
relationHasPrimaryKey(Relation rel)
@@ -1247,8 +1251,9 @@ index_constraint_create(Relation heapRelation,
12471251
* Note: since this is a transactional update, it's unsafe against
12481252
* concurrent SnapshotNow scans of pg_index. When making an existing
12491253
* index into a constraint, caller must have a table lock that prevents
1250-
* concurrent table updates, and there is a risk that concurrent readers
1251-
* of the table will miss seeing this index at all.
1254+
* concurrent table updates; if it's less than a full exclusive lock,
1255+
* there is a risk that concurrent readers of the table will miss seeing
1256+
* this index at all.
12521257
*/
12531258
if (update_pgindex && (mark_as_primary || deferrable))
12541259
{
@@ -1450,7 +1455,7 @@ BuildIndexInfo(Relation index)
14501455

14511456
/* other info */
14521457
ii->ii_Unique = indexStruct->indisunique;
1453-
ii->ii_ReadyForInserts = indexStruct->indisready;
1458+
ii->ii_ReadyForInserts = IndexIsReady(indexStruct);
14541459

14551460
/* initialize index-build state to default */
14561461
ii->ii_Concurrent = false;
@@ -1789,8 +1794,20 @@ index_build(Relation heapRelation,
17891794
* index's usability horizon. Moreover, we *must not* try to change the
17901795
* index's pg_index entry while reindexing pg_index itself, and this
17911796
* optimization nicely prevents that.
1792-
*/
1793-
if (indexInfo->ii_BrokenHotChain && !isreindex)
1797+
*
1798+
* We also need not set indcheckxmin during a concurrent index build,
1799+
* because we won't set indisvalid true until all transactions that care
1800+
* about the broken HOT chains are gone.
1801+
*
1802+
* Therefore, this code path can only be taken during non-concurrent
1803+
* CREATE INDEX. Thus the fact that heap_update will set the pg_index
1804+
* tuple's xmin doesn't matter, because that tuple was created in the
1805+
* current transaction anyway. That also means we don't need to worry
1806+
* about any concurrent readers of the tuple; no other transaction can see
1807+
* it yet.
1808+
*/
1809+
if (indexInfo->ii_BrokenHotChain && !isreindex &&
1810+
!indexInfo->ii_Concurrent)
17941811
{
17951812
Oid indexId = RelationGetRelid(indexRelation);
17961813
Relation pg_index;
@@ -2753,6 +2770,65 @@ validate_index_heapscan(Relation heapRelation,
27532770
}
27542771

27552772

2773+
/*
2774+
* index_set_state_flags - adjust pg_index state flags
2775+
*
2776+
* This is used during CREATE INDEX CONCURRENTLY to adjust the pg_index
2777+
* flags that denote the index's state. We must use an in-place update of
2778+
* the pg_index tuple, because we do not have exclusive lock on the parent
2779+
* table and so other sessions might concurrently be doing SnapshotNow scans
2780+
* of pg_index to identify the table's indexes. A transactional update would
2781+
* risk somebody not seeing the index at all. Because the update is not
2782+
* transactional and will not roll back on error, this must only be used as
2783+
* the last step in a transaction that has not made any transactional catalog
2784+
* updates!
2785+
*
2786+
* Note that heap_inplace_update does send a cache inval message for the
2787+
* tuple, so other sessions will hear about the update as soon as we commit.
2788+
*/
2789+
void
2790+
index_set_state_flags(Oid indexId, IndexStateFlagsAction action)
2791+
{
2792+
Relation pg_index;
2793+
HeapTuple indexTuple;
2794+
Form_pg_index indexForm;
2795+
2796+
/* Assert that current xact hasn't done any transactional updates */
2797+
Assert(GetTopTransactionIdIfAny() == InvalidTransactionId);
2798+
2799+
/* Open pg_index and fetch a writable copy of the index's tuple */
2800+
pg_index = heap_open(IndexRelationId, RowExclusiveLock);
2801+
2802+
indexTuple = SearchSysCacheCopy1(INDEXRELID,
2803+
ObjectIdGetDatum(indexId));
2804+
if (!HeapTupleIsValid(indexTuple))
2805+
elog(ERROR, "cache lookup failed for index %u", indexId);
2806+
indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
2807+
2808+
/* Perform the requested state change on the copy */
2809+
switch (action)
2810+
{
2811+
case INDEX_CREATE_SET_READY:
2812+
/* Set indisready during a CREATE INDEX CONCURRENTLY sequence */
2813+
Assert(!indexForm->indisready);
2814+
Assert(!indexForm->indisvalid);
2815+
indexForm->indisready = true;
2816+
break;
2817+
case INDEX_CREATE_SET_VALID:
2818+
/* Set indisvalid during a CREATE INDEX CONCURRENTLY sequence */
2819+
Assert(indexForm->indisready);
2820+
Assert(!indexForm->indisvalid);
2821+
indexForm->indisvalid = true;
2822+
break;
2823+
}
2824+
2825+
/* ... and write it back in-place */
2826+
heap_inplace_update(pg_index, indexTuple);
2827+
2828+
heap_close(pg_index, RowExclusiveLock);
2829+
}
2830+
2831+
27562832
/*
27572833
* IndexGetRelation: given an index's relation OID, get the OID of the
27582834
* relation it is an index on. Uses the system cache.
@@ -2782,12 +2858,9 @@ void
27822858
reindex_index(Oid indexId, bool skip_constraint_checks)
27832859
{
27842860
Relation iRel,
2785-
heapRelation,
2786-
pg_index;
2861+
heapRelation;
27872862
Oid heapId;
27882863
IndexInfo *indexInfo;
2789-
HeapTuple indexTuple;
2790-
Form_pg_index indexForm;
27912864
volatile bool skipped_constraint = false;
27922865

27932866
/*
@@ -2867,25 +2940,39 @@ reindex_index(Oid indexId, bool skip_constraint_checks)
28672940
*
28682941
* We can also reset indcheckxmin, because we have now done a
28692942
* non-concurrent index build, *except* in the case where index_build
2870-
* found some still-broken HOT chains. If it did, we normally leave
2871-
* indcheckxmin alone (note that index_build won't have changed it,
2872-
* because this is a reindex). But if the index was invalid or not ready
2873-
* and there were broken HOT chains, it seems best to force indcheckxmin
2874-
* true, because the normal argument that the HOT chains couldn't conflict
2875-
* with the index is suspect for an invalid index.
2943+
* found some still-broken HOT chains. If it did, and we don't have to
2944+
* change any of the other flags, we just leave indcheckxmin alone (note
2945+
* that index_build won't have changed it, because this is a reindex).
2946+
* This is okay and desirable because not updating the tuple leaves the
2947+
* index's usability horizon (recorded as the tuple's xmin value) the same
2948+
* as it was.
2949+
*
2950+
* But, if the index was invalid/not-ready and there were broken HOT
2951+
* chains, we had better force indcheckxmin true, because the normal
2952+
* argument that the HOT chains couldn't conflict with the index is
2953+
* suspect for an invalid index. In this case advancing the usability
2954+
* horizon is appropriate.
2955+
*
2956+
* Note that if we have to update the tuple, there is a risk of concurrent
2957+
* transactions not seeing it during their SnapshotNow scans of pg_index.
2958+
* While not especially desirable, this is safe because no such
2959+
* transaction could be trying to update the table (since we have
2960+
* ShareLock on it). The worst case is that someone might transiently
2961+
* fail to use the index for a query --- but it was probably unusable
2962+
* before anyway, if we are updating the tuple.
28762963
*
2877-
* Note that it is important to not update the pg_index entry if we don't
2878-
* have to, because updating it will move the index's usability horizon
2879-
* (recorded as the tuple's xmin value) if indcheckxmin is true. We don't
2880-
* really want REINDEX to move the usability horizon forward ever, but we
2881-
* have no choice if we are to fix indisvalid or indisready. Of course,
2882-
* clearing indcheckxmin eliminates the issue, so we're happy to do that
2883-
* if we can. Another reason for caution here is that while reindexing
2884-
* pg_index itself, we must not try to update it. We assume that
2885-
* pg_index's indexes will always have these flags in their clean state.
2964+
* Another reason for avoiding unnecessary updates here is that while
2965+
* reindexing pg_index itself, we must not try to update tuples in it.
2966+
* pg_index's indexes should always have these flags in their clean state,
2967+
* so that won't happen.
28862968
*/
28872969
if (!skipped_constraint)
28882970
{
2971+
Relation pg_index;
2972+
HeapTuple indexTuple;
2973+
Form_pg_index indexForm;
2974+
bool index_bad;
2975+
28892976
pg_index = heap_open(IndexRelationId, RowExclusiveLock);
28902977

28912978
indexTuple = SearchSysCacheCopy1(INDEXRELID,
@@ -2894,17 +2981,28 @@ reindex_index(Oid indexId, bool skip_constraint_checks)
28942981
elog(ERROR, "cache lookup failed for index %u", indexId);
28952982
indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
28962983

2897-
if (!indexForm->indisvalid || !indexForm->indisready ||
2984+
index_bad = (!indexForm->indisvalid ||
2985+
!indexForm->indisready);
2986+
if (index_bad ||
28982987
(indexForm->indcheckxmin && !indexInfo->ii_BrokenHotChain))
28992988
{
29002989
if (!indexInfo->ii_BrokenHotChain)
29012990
indexForm->indcheckxmin = false;
2902-
else if (!indexForm->indisvalid || !indexForm->indisready)
2991+
else if (index_bad)
29032992
indexForm->indcheckxmin = true;
29042993
indexForm->indisvalid = true;
29052994
indexForm->indisready = true;
29062995
simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
29072996
CatalogUpdateIndexes(pg_index, indexTuple);
2997+
2998+
/*
2999+
* Invalidate the relcache for the table, so that after we commit
3000+
* all sessions will refresh the table's index list. This ensures
3001+
* that if anyone misses seeing the pg_index row during this
3002+
* update, they'll refresh their list before attempting any update
3003+
* on the table.
3004+
*/
3005+
CacheInvalidateRelcache(heapRelation);
29083006
}
29093007

29103008
heap_close(pg_index, RowExclusiveLock);

‎src/backend/commands/cluster.c

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -454,7 +454,7 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMOD
454454
* might put recently-dead tuples out-of-order in the new table, and there
455455
* is little harm in that.)
456456
*/
457-
if (!OldIndex->rd_index->indisvalid)
457+
if (!IndexIsValid(OldIndex->rd_index))
458458
ereport(ERROR,
459459
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
460460
errmsg("cannot cluster on invalid index \"%s\"",
@@ -468,6 +468,11 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMOD
468468
* mark_index_clustered: mark the specified index as the one clustered on
469469
*
470470
* With indexOid == InvalidOid, will mark all indexes of rel not-clustered.
471+
*
472+
* Note: we do transactional updates of the pg_index rows, which are unsafe
473+
* against concurrent SnapshotNow scans of pg_index. Therefore this is unsafe
474+
* to execute with less than full exclusive lock on the parent table;
475+
* otherwise concurrent executions of RelationGetIndexList could miss indexes.
471476
*/
472477
void
473478
mark_index_clustered(Relation rel, Oid indexOid)
@@ -523,6 +528,9 @@ mark_index_clustered(Relation rel, Oid indexOid)
523528
}
524529
else if (thisIndexOid == indexOid)
525530
{
531+
/* this was checked earlier, but let's be real sure */
532+
if (!IndexIsValid(indexForm))
533+
elog(ERROR, "cannot cluster on invalid index %u", indexOid);
526534
indexForm->indisclustered = true;
527535
simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
528536
CatalogUpdateIndexes(pg_index, indexTuple);

‎src/backend/commands/indexcmds.c

Lines changed: 3 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -148,9 +148,6 @@ DefineIndex(RangeVar *heapRelation,
148148
LockRelId heaprelid;
149149
LOCKTAG heaplocktag;
150150
Snapshot snapshot;
151-
Relation pg_index;
152-
HeapTuple indexTuple;
153-
Form_pg_index indexForm;
154151
int i;
155152

156153
/*
@@ -531,23 +528,7 @@ DefineIndex(RangeVar *heapRelation,
531528
* commit this transaction, any new transactions that open the table must
532529
* insert new entries into the index for insertions and non-HOT updates.
533530
*/
534-
pg_index = heap_open(IndexRelationId, RowExclusiveLock);
535-
536-
indexTuple = SearchSysCacheCopy1(INDEXRELID,
537-
ObjectIdGetDatum(indexRelationId));
538-
if (!HeapTupleIsValid(indexTuple))
539-
elog(ERROR, "cache lookup failed for index %u", indexRelationId);
540-
indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
541-
542-
Assert(!indexForm->indisready);
543-
Assert(!indexForm->indisvalid);
544-
545-
indexForm->indisready = true;
546-
547-
simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
548-
CatalogUpdateIndexes(pg_index, indexTuple);
549-
550-
heap_close(pg_index, RowExclusiveLock);
531+
index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
551532

552533
/* we can do away with our snapshot */
553534
PopActiveSnapshot();
@@ -671,31 +652,15 @@ DefineIndex(RangeVar *heapRelation,
671652
/*
672653
* Index can now be marked valid -- update its pg_index entry
673654
*/
674-
pg_index = heap_open(IndexRelationId, RowExclusiveLock);
675-
676-
indexTuple = SearchSysCacheCopy1(INDEXRELID,
677-
ObjectIdGetDatum(indexRelationId));
678-
if (!HeapTupleIsValid(indexTuple))
679-
elog(ERROR, "cache lookup failed for index %u", indexRelationId);
680-
indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
681-
682-
Assert(indexForm->indisready);
683-
Assert(!indexForm->indisvalid);
684-
685-
indexForm->indisvalid = true;
686-
687-
simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
688-
CatalogUpdateIndexes(pg_index, indexTuple);
689-
690-
heap_close(pg_index, RowExclusiveLock);
655+
index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
691656

692657
/*
693658
* The pg_index update will cause backends (including this one) to update
694659
* relcache entries for the index itself, but we should also send a
695660
* relcache inval on the parent table to force replanning of cached plans.
696661
* Otherwise existing sessions might fail to use the new index where it
697662
* would be useful. (Note that our earlier commits did not create reasons
698-
* to replan; relcache flush on the index itself was sufficient.)
663+
* to replan; so relcache flush on the index itself was sufficient.)
699664
*/
700665
CacheInvalidateRelcacheByRelid(heaprelid.relId);
701666

‎src/backend/commands/tablecmds.c

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4528,6 +4528,8 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
45284528

45294529
/*
45304530
* Check that the attribute is not in a primary key
4531+
*
4532+
* Note: we'll throw error even if the pkey index is not valid.
45314533
*/
45324534

45334535
/* Loop over all indexes on the relation */
@@ -5876,7 +5878,7 @@ transformFkeyGetPrimaryKey(Relation pkrel, Oid *indexOid,
58765878
/*
58775879
* Get the list of index OIDs for the table from the relcache, and look up
58785880
* each one in the pg_index syscache until we find one marked primary key
5879-
* (hopefully there isn't more than one such).
5881+
* (hopefully there isn't more than one such). Insist it's valid, too.
58805882
*/
58815883
*indexOid = InvalidOid;
58825884

@@ -5890,7 +5892,7 @@ transformFkeyGetPrimaryKey(Relation pkrel, Oid *indexOid,
58905892
if (!HeapTupleIsValid(indexTuple))
58915893
elog(ERROR, "cache lookup failed for index %u", indexoid);
58925894
indexStruct = (Form_pg_index) GETSTRUCT(indexTuple);
5893-
if (indexStruct->indisprimary)
5895+
if (indexStruct->indisprimary && IndexIsValid(indexStruct))
58945896
{
58955897
/*
58965898
* Refuse to use a deferrable primary key. This is per SQL spec,
@@ -5988,10 +5990,12 @@ transformFkeyCheckAttrs(Relation pkrel,
59885990

59895991
/*
59905992
* Must have the right number of columns; must be unique and not a
5991-
* partial index; forget it if there are any expressions, too
5993+
* partial index; forget it if there are any expressions, too. Invalid
5994+
* indexes are out as well.
59925995
*/
59935996
if (indexStruct->indnatts == numattrs &&
59945997
indexStruct->indisunique &&
5998+
IndexIsValid(indexStruct) &&
59955999
heap_attisnull(indexTuple, Anum_pg_index_indpred) &&
59966000
heap_attisnull(indexTuple, Anum_pg_index_indexprs))
59976001
{

0 commit comments

Comments
 (0)