Skip to content

Commit 3c84046

Browse files
tglsfdctglsfdc
committed
Fix assorted bugs in CREATE/DROP INDEX CONCURRENTLY.
Commit 8cb5365, which introduced DROP INDEX CONCURRENTLY, managed to break CREATE INDEX CONCURRENTLY via a poor choice of catalog state representation. The pg_index state for an index that's reached the final pre-drop stage was the same as the state for an index just created by CREATE INDEX CONCURRENTLY. This meant that the (necessary) change to make RelationGetIndexList ignore about-to-die indexes also made it ignore freshly-created indexes; which is catastrophic because the latter do need to be considered in HOT-safety decisions. Failure to do so leads to incorrect index entries and subsequently wrong results from queries depending on the concurrently-created index. To fix, add an additional boolean column "indislive" to pg_index, so that the freshly-created and about-to-die states can be distinguished. (This change obviously is only possible in HEAD. This patch will need to be back-patched, but in 9.2 we'll use a kluge consisting of overloading the formerly-impossible state of indisvalid = true and indisready = false.) In addition, change CREATE/DROP INDEX CONCURRENTLY so that the pg_index flag changes they make 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. This is a pre-existing bug in CREATE INDEX CONCURRENTLY, which was copied into the DROP code. 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. In addition, do some code and docs review for DROP INDEX CONCURRENTLY; some cosmetic code cleanup but mostly addition and revision of comments. This will need to be back-patched, but in a noticeably different form, so I'm committing it to HEAD before working on the back-patch. Problem reported by Amit Kapila, diagnosis by Pavan Deolassee, fix by Tom Lane and Andres Freund.
1 parent 1577b46 commit 3c84046

File tree

17 files changed

+464
-265
lines changed

17 files changed

+464
-265
lines changed

‎contrib/tcn/tcn.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,8 @@ triggered_change_notification(PG_FUNCTION_ARGS)
141141
if (!HeapTupleIsValid(indexTuple)) /* should not happen */
142142
elog(ERROR, "cache lookup failed for index %u", indexoid);
143143
index = (Form_pg_index) GETSTRUCT(indexTuple);
144-
/* we're only interested if it is the primary key */
145-
if (index->indisprimary)
144+
/* we're only interested if it is the primary key and valid */
145+
if (index->indisprimary && IndexIsValid(index))
146146
{
147147
int numatts = index->indnatts;
148148

‎doc/src/sgml/catalogs.sgml

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3480,7 +3480,7 @@
34803480
index is possibly incomplete: it must still be modified by
34813481
<command>INSERT</>/<command>UPDATE</> operations, but it cannot safely
34823482
be used for queries. If it is unique, the uniqueness property is not
3483-
true either.
3483+
guaranteed true either.
34843484
</entry>
34853485
</row>
34863486

@@ -3507,6 +3507,16 @@
35073507
</entry>
35083508
</row>
35093509

3510+
<row>
3511+
<entry><structfield>indislive</structfield></entry>
3512+
<entry><type>bool</type></entry>
3513+
<entry></entry>
3514+
<entry>
3515+
If false, the index is in process of being dropped, and should be
3516+
ignored for all purposes (including HOT-safety decisions)
3517+
</entry>
3518+
</row>
3519+
35103520
<row>
35113521
<entry><structfield>indkey</structfield></entry>
35123522
<entry><type>int2vector</type></entry>

‎doc/src/sgml/ref/drop_index.sgml

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -40,34 +40,33 @@ DROP INDEX [ CONCURRENTLY ] [ IF EXISTS ] <replaceable class="PARAMETER">name</r
4040

4141
<variablelist>
4242
<varlistentry>
43-
<term><literal>IF EXISTS</literal></term>
43+
<term><literal>CONCURRENTLY</literal></term>
4444
<listitem>
4545
<para>
46-
Do not throw an error if the index does not exist. A notice is issued
47-
in this case.
46+
Drop the index without locking out concurrent selects, inserts, updates,
47+
and deletes on the index's table. A normal <command>DROP INDEX</>
48+
acquires exclusive lock on the table, blocking other accesses until the
49+
index drop can be completed. With this option, the command instead
50+
waits until conflicting transactions have completed.
51+
</para>
52+
<para>
53+
There are several caveats to be aware of when using this option.
54+
Only one index name can be specified, and the <literal>CASCADE</> option
55+
is not supported. (Thus, an index that supports a <literal>UNIQUE</> or
56+
<literal>PRIMARY KEY</> constraint cannot be dropped this way.)
57+
Also, regular <command>DROP INDEX</> commands can be
58+
performed within a transaction block, but
59+
<command>DROP INDEX CONCURRENTLY</> cannot.
4860
</para>
4961
</listitem>
5062
</varlistentry>
5163

5264
<varlistentry>
53-
<term><literal>CONCURRENTLY</literal></term>
65+
<term><literal>IF EXISTS</literal></term>
5466
<listitem>
5567
<para>
56-
When this option is used, <productname>PostgreSQL</> will drop the
57-
index without taking any locks that prevent concurrent selects, inserts,
58-
updates, or deletes on the table; whereas a standard index drop
59-
waits for a lock that locks out everything on the table until it's done.
60-
Concurrent drop index is a two stage process. First, we mark the index
61-
both invalid and not ready then commit the change. Next we wait until
62-
there are no users locking the table who can see the index.
63-
</para>
64-
<para>
65-
There are several caveats to be aware of when using this option.
66-
Only one index name can be specified if the <literal>CONCURRENTLY</literal>
67-
parameter is specified. Regular <command>DROP INDEX</> command can be
68-
performed within a transaction block, but
69-
<command>DROP INDEX CONCURRENTLY</> cannot.
70-
The CASCADE option is not supported when dropping an index concurrently.
68+
Do not throw an error if the index does not exist. A notice is issued
69+
in this case.
7170
</para>
7271
</listitem>
7372
</varlistentry>

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,34 @@ 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+
395+
396+
DROP INDEX CONCURRENTLY
397+
-----------------------
398+
399+
DROP INDEX CONCURRENTLY is sort of the reverse sequence of CREATE INDEX
400+
CONCURRENTLY. We first mark the index as not indisvalid, and then wait for
401+
any transactions that could be using it in queries to end. (During this
402+
time, index updates must still be performed as normal, since such
403+
transactions might expect freshly inserted tuples to be findable.)
404+
Then, we clear indisready and indislive, and again wait for transactions
405+
that could be updating the index to end. Finally we can drop the index
406+
normally (though taking only ShareUpdateExclusiveLock on its parent table).
407+
408+
The reason we need the pg_index.indislive flag is that after the second
409+
wait step begins, we don't want transactions to be touching the index at
410+
all; otherwise they might suffer errors if the DROP finally commits while
411+
they are reading catalog entries for the index. If we had only indisvalid
412+
and indisready, this state would be indistinguishable from the first stage
413+
of CREATE INDEX CONCURRENTLY --- but in that state, we *do* want
414+
transactions to examine the index, since they must consider it in
415+
HOT-safety checks.
416+
389417

390418
Limitations and Restrictions
391419
----------------------------

‎src/backend/catalog/dependency.c

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -995,7 +995,6 @@ deleteOneObject(const ObjectAddress *object, Relation depRel, int flags)
995995
int nkeys;
996996
SysScanDesc scan;
997997
HeapTuple tup;
998-
Oid depRelOid = depRel->rd_id;
999998

1000999
/* DROP hook of the objects being removed */
10011000
if (object_access_hook)
@@ -1008,34 +1007,33 @@ deleteOneObject(const ObjectAddress *object, Relation depRel, int flags)
10081007
}
10091008

10101009
/*
1011-
* Close depRel if we are doing a drop concurrently. The individual
1012-
* deletion has to commit the transaction and we don't want dangling
1013-
* references.
1010+
* Close depRel if we are doing a drop concurrently. The object deletion
1011+
* subroutine will commit the current transaction, so we can't keep the
1012+
* relation open across doDeletion().
10141013
*/
10151014
if (flags & PERFORM_DELETION_CONCURRENTLY)
10161015
heap_close(depRel, RowExclusiveLock);
10171016

10181017
/*
10191018
* Delete the object itself, in an object-type-dependent way.
10201019
*
1021-
* Do this before removing outgoing dependencies as deletions can be
1022-
* happening in concurrent mode. That will only happen for a single object
1023-
* at once and if so the object will be invalidated inside a separate
1024-
* transaction and only dropped inside a transaction thats in-progress when
1025-
* doDeletion returns. This way no observer can see dangling dependency
1026-
* entries.
1020+
* We used to do this after removing the outgoing dependency links, but it
1021+
* seems just as reasonable to do it beforehand. In the concurrent case
1022+
* we *must* do it in this order, because we can't make any transactional
1023+
* updates before calling doDeletion() --- they'd get committed right
1024+
* away, which is not cool if the deletion then fails.
10271025
*/
10281026
doDeletion(object, flags);
10291027

10301028
/*
1031-
* Reopen depRel if we closed it before
1029+
* Reopen depRel if we closed it above
10321030
*/
10331031
if (flags & PERFORM_DELETION_CONCURRENTLY)
1034-
depRel = heap_open(depRelOid, RowExclusiveLock);
1032+
depRel = heap_open(DependRelationId, RowExclusiveLock);
10351033

10361034
/*
1037-
* Then remove any pg_depend records that link from this object to
1038-
* others. (Any records linking to this object should be gone already.)
1035+
* Now remove any pg_depend records that link from this object to others.
1036+
* (Any records linking to this object should be gone already.)
10391037
*
10401038
* When dropping a whole object (subId = 0), remove all pg_depend records
10411039
* for its sub-objects too.
@@ -1258,15 +1256,23 @@ AcquireDeletionLock(const ObjectAddress *object, int flags)
12581256
{
12591257
if (object->classId == RelationRelationId)
12601258
{
1259+
/*
1260+
* In DROP INDEX CONCURRENTLY, take only ShareUpdateExclusiveLock on
1261+
* the index for the moment. index_drop() will promote the lock once
1262+
* it's safe to do so. In all other cases we need full exclusive
1263+
* lock.
1264+
*/
12611265
if (flags & PERFORM_DELETION_CONCURRENTLY)
12621266
LockRelationOid(object->objectId, ShareUpdateExclusiveLock);
12631267
else
12641268
LockRelationOid(object->objectId, AccessExclusiveLock);
12651269
}
12661270
else
1271+
{
12671272
/* assume we should lock the whole object not a sub-object */
12681273
LockDatabaseObject(object->classId, object->objectId, 0,
12691274
AccessExclusiveLock);
1275+
}
12701276
}
12711277

12721278
/*

0 commit comments

Comments
 (0)