Skip to content

Commit 1dbd02d

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 3dfdf28 commit 1dbd02d

File tree

11 files changed

+207
-76
lines changed

11 files changed

+207
-76
lines changed

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

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

383+
Note that we do not need to set pg_index.indcheckxmin in this code path,
384+
because we have outwaited any transactions that would need to avoid using
385+
the index. (indcheckxmin is only needed because non-concurrent CREATE
386+
INDEX doesn't want to wait; its stronger lock would create too much risk of
387+
deadlock if it did.)
388+
383389

384390
Limitations and Restrictions
385391
----------------------------

‎src/backend/catalog/index.c

Lines changed: 117 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1126,7 +1126,7 @@ BuildIndexInfo(Relation index)
11261126

11271127
/* other info */
11281128
ii->ii_Unique = indexStruct->indisunique;
1129-
ii->ii_ReadyForInserts = indexStruct->indisready;
1129+
ii->ii_ReadyForInserts = IndexIsReady(indexStruct);
11301130

11311131
/* initialize index-build state to default */
11321132
ii->ii_Concurrent = false;
@@ -1465,8 +1465,20 @@ index_build(Relation heapRelation,
14651465
* index's usability horizon. Moreover, we *must not* try to change
14661466
* the index's pg_index entry while reindexing pg_index itself, and this
14671467
* optimization nicely prevents that.
1468-
*/
1469-
if (indexInfo->ii_BrokenHotChain && !isreindex)
1468+
*
1469+
* We also need not set indcheckxmin during a concurrent index build,
1470+
* because we won't set indisvalid true until all transactions that care
1471+
* about the broken HOT chains are gone.
1472+
*
1473+
* Therefore, this code path can only be taken during non-concurrent
1474+
* CREATE INDEX. Thus the fact that heap_update will set the pg_index
1475+
* tuple's xmin doesn't matter, because that tuple was created in the
1476+
* current transaction anyway. That also means we don't need to worry
1477+
* about any concurrent readers of the tuple; no other transaction can see
1478+
* it yet.
1479+
*/
1480+
if (indexInfo->ii_BrokenHotChain && !isreindex &&
1481+
!indexInfo->ii_Concurrent)
14701482
{
14711483
Oid indexId = RelationGetRelid(indexRelation);
14721484
Relation pg_index;
@@ -2408,6 +2420,65 @@ validate_index_heapscan(Relation heapRelation,
24082420
}
24092421

24102422

2423+
/*
2424+
* index_set_state_flags - adjust pg_index state flags
2425+
*
2426+
* This is used during CREATE INDEX CONCURRENTLY to adjust the pg_index
2427+
* flags that denote the index's state. We must use an in-place update of
2428+
* the pg_index tuple, because we do not have exclusive lock on the parent
2429+
* table and so other sessions might concurrently be doing SnapshotNow scans
2430+
* of pg_index to identify the table's indexes. A transactional update would
2431+
* risk somebody not seeing the index at all. Because the update is not
2432+
* transactional and will not roll back on error, this must only be used as
2433+
* the last step in a transaction that has not made any transactional catalog
2434+
* updates!
2435+
*
2436+
* Note that heap_inplace_update does send a cache inval message for the
2437+
* tuple, so other sessions will hear about the update as soon as we commit.
2438+
*/
2439+
void
2440+
index_set_state_flags(Oid indexId, IndexStateFlagsAction action)
2441+
{
2442+
Relation pg_index;
2443+
HeapTuple indexTuple;
2444+
Form_pg_index indexForm;
2445+
2446+
/* Assert that current xact hasn't done any transactional updates */
2447+
Assert(GetTopTransactionIdIfAny() == InvalidTransactionId);
2448+
2449+
/* Open pg_index and fetch a writable copy of the index's tuple */
2450+
pg_index = heap_open(IndexRelationId, RowExclusiveLock);
2451+
2452+
indexTuple = SearchSysCacheCopy1(INDEXRELID,
2453+
ObjectIdGetDatum(indexId));
2454+
if (!HeapTupleIsValid(indexTuple))
2455+
elog(ERROR, "cache lookup failed for index %u", indexId);
2456+
indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
2457+
2458+
/* Perform the requested state change on the copy */
2459+
switch (action)
2460+
{
2461+
case INDEX_CREATE_SET_READY:
2462+
/* Set indisready during a CREATE INDEX CONCURRENTLY sequence */
2463+
Assert(!indexForm->indisready);
2464+
Assert(!indexForm->indisvalid);
2465+
indexForm->indisready = true;
2466+
break;
2467+
case INDEX_CREATE_SET_VALID:
2468+
/* Set indisvalid during a CREATE INDEX CONCURRENTLY sequence */
2469+
Assert(indexForm->indisready);
2470+
Assert(!indexForm->indisvalid);
2471+
indexForm->indisvalid = true;
2472+
break;
2473+
}
2474+
2475+
/* ... and write it back in-place */
2476+
heap_inplace_update(pg_index, indexTuple);
2477+
2478+
heap_close(pg_index, RowExclusiveLock);
2479+
}
2480+
2481+
24112482
/*
24122483
* IndexGetRelation: given an index's relation OID, get the OID of the
24132484
* relation it is an index on. Uses the system cache.
@@ -2437,12 +2508,9 @@ void
24372508
reindex_index(Oid indexId, bool skip_constraint_checks)
24382509
{
24392510
Relation iRel,
2440-
heapRelation,
2441-
pg_index;
2511+
heapRelation;
24422512
Oid heapId;
24432513
IndexInfo *indexInfo;
2444-
HeapTuple indexTuple;
2445-
Form_pg_index indexForm;
24462514
volatile bool skipped_constraint = false;
24472515

24482516
/*
@@ -2516,25 +2584,39 @@ reindex_index(Oid indexId, bool skip_constraint_checks)
25162584
*
25172585
* We can also reset indcheckxmin, because we have now done a
25182586
* non-concurrent index build, *except* in the case where index_build
2519-
* found some still-broken HOT chains. If it did, we normally leave
2520-
* indcheckxmin alone (note that index_build won't have changed it,
2521-
* because this is a reindex). But if the index was invalid or not ready
2522-
* and there were broken HOT chains, it seems best to force indcheckxmin
2523-
* true, because the normal argument that the HOT chains couldn't conflict
2524-
* with the index is suspect for an invalid index.
2587+
* found some still-broken HOT chains. If it did, and we don't have to
2588+
* change any of the other flags, we just leave indcheckxmin alone (note
2589+
* that index_build won't have changed it, because this is a reindex).
2590+
* This is okay and desirable because not updating the tuple leaves the
2591+
* index's usability horizon (recorded as the tuple's xmin value) the same
2592+
* as it was.
25252593
*
2526-
* Note that it is important to not update the pg_index entry if we don't
2527-
* have to, because updating it will move the index's usability horizon
2528-
* (recorded as the tuple's xmin value) if indcheckxmin is true. We don't
2529-
* really want REINDEX to move the usability horizon forward ever, but we
2530-
* have no choice if we are to fix indisvalid or indisready. Of course,
2531-
* clearing indcheckxmin eliminates the issue, so we're happy to do that
2532-
* if we can. Another reason for caution here is that while reindexing
2533-
* pg_index itself, we must not try to update it. We assume that
2534-
* pg_index's indexes will always have these flags in their clean state.
2594+
* But, if the index was invalid/not-ready and there were broken HOT
2595+
* chains, we had better force indcheckxmin true, because the normal
2596+
* argument that the HOT chains couldn't conflict with the index is
2597+
* suspect for an invalid index. In this case advancing the usability
2598+
* horizon is appropriate.
2599+
*
2600+
* Note that if we have to update the tuple, there is a risk of concurrent
2601+
* transactions not seeing it during their SnapshotNow scans of pg_index.
2602+
* While not especially desirable, this is safe because no such
2603+
* transaction could be trying to update the table (since we have
2604+
* ShareLock on it). The worst case is that someone might transiently
2605+
* fail to use the index for a query --- but it was probably unusable
2606+
* before anyway, if we are updating the tuple.
2607+
*
2608+
* Another reason for avoiding unnecessary updates here is that while
2609+
* reindexing pg_index itself, we must not try to update tuples in it.
2610+
* pg_index's indexes should always have these flags in their clean state,
2611+
* so that won't happen.
25352612
*/
25362613
if (!skipped_constraint)
25372614
{
2615+
Relation pg_index;
2616+
HeapTuple indexTuple;
2617+
Form_pg_index indexForm;
2618+
bool index_bad;
2619+
25382620
pg_index = heap_open(IndexRelationId, RowExclusiveLock);
25392621

25402622
indexTuple = SearchSysCacheCopy1(INDEXRELID,
@@ -2543,17 +2625,28 @@ reindex_index(Oid indexId, bool skip_constraint_checks)
25432625
elog(ERROR, "cache lookup failed for index %u", indexId);
25442626
indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
25452627

2546-
if (!indexForm->indisvalid || !indexForm->indisready ||
2628+
index_bad = (!indexForm->indisvalid ||
2629+
!indexForm->indisready);
2630+
if (index_bad ||
25472631
(indexForm->indcheckxmin && !indexInfo->ii_BrokenHotChain))
25482632
{
25492633
if (!indexInfo->ii_BrokenHotChain)
25502634
indexForm->indcheckxmin = false;
2551-
else if (!indexForm->indisvalid || !indexForm->indisready)
2635+
else if (index_bad)
25522636
indexForm->indcheckxmin = true;
25532637
indexForm->indisvalid = true;
25542638
indexForm->indisready = true;
25552639
simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
25562640
CatalogUpdateIndexes(pg_index, indexTuple);
2641+
2642+
/*
2643+
* Invalidate the relcache for the table, so that after we commit
2644+
* all sessions will refresh the table's index list. This ensures
2645+
* that if anyone misses seeing the pg_index row during this
2646+
* update, they'll refresh their list before attempting any update
2647+
* on the table.
2648+
*/
2649+
CacheInvalidateRelcache(heapRelation);
25572650
}
25582651

25592652
heap_close(pg_index, RowExclusiveLock);

‎src/backend/commands/cluster.c

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -487,7 +487,7 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck)
487487
* might put recently-dead tuples out-of-order in the new table, and there
488488
* is little harm in that.)
489489
*/
490-
if (!OldIndex->rd_index->indisvalid)
490+
if (!IndexIsValid(OldIndex->rd_index))
491491
ereport(ERROR,
492492
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
493493
errmsg("cannot cluster on invalid index \"%s\"",
@@ -501,6 +501,11 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck)
501501
* mark_index_clustered: mark the specified index as the one clustered on
502502
*
503503
* With indexOid == InvalidOid, will mark all indexes of rel not-clustered.
504+
*
505+
* Note: we do transactional updates of the pg_index rows, which are unsafe
506+
* against concurrent SnapshotNow scans of pg_index. Therefore this is unsafe
507+
* to execute with less than full exclusive lock on the parent table;
508+
* otherwise concurrent executions of RelationGetIndexList could miss indexes.
504509
*/
505510
void
506511
mark_index_clustered(Relation rel, Oid indexOid)
@@ -556,6 +561,9 @@ mark_index_clustered(Relation rel, Oid indexOid)
556561
}
557562
else if (thisIndexOid == indexOid)
558563
{
564+
/* this was checked earlier, but let's be real sure */
565+
if (!IndexIsValid(indexForm))
566+
elog(ERROR, "cannot cluster on invalid index %u", indexOid);
559567
indexForm->indisclustered = true;
560568
simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
561569
CatalogUpdateIndexes(pg_index, indexTuple);

‎src/backend/commands/indexcmds.c

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

155152
/*
@@ -595,23 +592,7 @@ DefineIndex(RangeVar *heapRelation,
595592
* commit this transaction, any new transactions that open the table must
596593
* insert new entries into the index for insertions and non-HOT updates.
597594
*/
598-
pg_index = heap_open(IndexRelationId, RowExclusiveLock);
599-
600-
indexTuple = SearchSysCacheCopy1(INDEXRELID,
601-
ObjectIdGetDatum(indexRelationId));
602-
if (!HeapTupleIsValid(indexTuple))
603-
elog(ERROR, "cache lookup failed for index %u", indexRelationId);
604-
indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
605-
606-
Assert(!indexForm->indisready);
607-
Assert(!indexForm->indisvalid);
608-
609-
indexForm->indisready = true;
610-
611-
simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
612-
CatalogUpdateIndexes(pg_index, indexTuple);
613-
614-
heap_close(pg_index, RowExclusiveLock);
595+
index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
615596

616597
/* we can do away with our snapshot */
617598
PopActiveSnapshot();
@@ -735,31 +716,15 @@ DefineIndex(RangeVar *heapRelation,
735716
/*
736717
* Index can now be marked valid -- update its pg_index entry
737718
*/
738-
pg_index = heap_open(IndexRelationId, RowExclusiveLock);
739-
740-
indexTuple = SearchSysCacheCopy1(INDEXRELID,
741-
ObjectIdGetDatum(indexRelationId));
742-
if (!HeapTupleIsValid(indexTuple))
743-
elog(ERROR, "cache lookup failed for index %u", indexRelationId);
744-
indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
745-
746-
Assert(indexForm->indisready);
747-
Assert(!indexForm->indisvalid);
748-
749-
indexForm->indisvalid = true;
750-
751-
simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
752-
CatalogUpdateIndexes(pg_index, indexTuple);
753-
754-
heap_close(pg_index, RowExclusiveLock);
719+
index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
755720

756721
/*
757722
* The pg_index update will cause backends (including this one) to update
758723
* relcache entries for the index itself, but we should also send a
759724
* relcache inval on the parent table to force replanning of cached plans.
760725
* Otherwise existing sessions might fail to use the new index where it
761726
* would be useful. (Note that our earlier commits did not create reasons
762-
* to replan; relcache flush on the index itself was sufficient.)
727+
* to replan; so relcache flush on the index itself was sufficient.)
763728
*/
764729
CacheInvalidateRelcacheByRelid(heaprelid.relId);
765730

‎src/backend/commands/tablecmds.c

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

38833883
/*
38843884
* Check that the attribute is not in a primary key
3885+
*
3886+
* Note: we'll throw error even if the pkey index is not valid.
38853887
*/
38863888

38873889
/* Loop over all indexes on the relation */
@@ -5064,7 +5066,7 @@ transformFkeyGetPrimaryKey(Relation pkrel, Oid *indexOid,
50645066
/*
50655067
* Get the list of index OIDs for the table from the relcache, and look up
50665068
* each one in the pg_index syscache until we find one marked primary key
5067-
* (hopefully there isn't more than one such).
5069+
* (hopefully there isn't more than one such). Insist it's valid, too.
50685070
*/
50695071
*indexOid = InvalidOid;
50705072

@@ -5078,7 +5080,7 @@ transformFkeyGetPrimaryKey(Relation pkrel, Oid *indexOid,
50785080
if (!HeapTupleIsValid(indexTuple))
50795081
elog(ERROR, "cache lookup failed for index %u", indexoid);
50805082
indexStruct = (Form_pg_index) GETSTRUCT(indexTuple);
5081-
if (indexStruct->indisprimary)
5083+
if (indexStruct->indisprimary && IndexIsValid(indexStruct))
50825084
{
50835085
/*
50845086
* Refuse to use a deferrable primary key. This is per SQL spec,
@@ -5176,10 +5178,12 @@ transformFkeyCheckAttrs(Relation pkrel,
51765178

51775179
/*
51785180
* Must have the right number of columns; must be unique and not a
5179-
* partial index; forget it if there are any expressions, too
5181+
* partial index; forget it if there are any expressions, too. Invalid
5182+
* indexes are out as well.
51805183
*/
51815184
if (indexStruct->indnatts == numattrs &&
51825185
indexStruct->indisunique &&
5186+
IndexIsValid(indexStruct) &&
51835187
heap_attisnull(indexTuple, Anum_pg_index_indpred) &&
51845188
heap_attisnull(indexTuple, Anum_pg_index_indexprs))
51855189
{

0 commit comments

Comments
 (0)