Skip to content

Commit d76652e

Browse files
michaelpqmichaelpq
committed
Fix concurrent indexing operations with temporary tables
Attempting to use CREATE INDEX, DROP INDEX or REINDEX with CONCURRENTLY on a temporary relation with ON COMMIT actions triggered unexpected errors because those operations use multiple transactions internally to complete their work. Here is for example one confusing error when using ON COMMIT DELETE ROWS: ERROR: index "foo" already contains data Issues related to temporary relations and concurrent indexing are fixed in this commit by enforcing the non-concurrent path to be taken for temporary relations even if using CONCURRENTLY, transparently to the user. Using a non-concurrent path does not matter in practice as locks cannot be taken on a temporary relation by a session different than the one owning the relation, and the non-concurrent operation is more effective. The problem exists with REINDEX since v12 with the introduction of CONCURRENTLY, and with CREATE/DROP INDEX since CONCURRENTLY exists for those commands. In all supported versions, this caused only confusing error messages to be generated. Note that with REINDEX, it was also possible to issue a REINDEX CONCURRENTLY for a temporary relation owned by a different session, leading to a server crash. The idea to enforce transparently the non-concurrent code path for temporary relations comes originally from Andres Freund. Reported-by: Manuel Rigger Author: Michael Paquier, Heikki Linnakangas Reviewed-by: Andres Freund, Álvaro Herrera, Heikki Linnakangas Discussion: https://postgr.es/m/CA+u7OA6gP7YAeCguyseusYcc=uR8+ypjCcgDDCTzjQ+k6S9ksQ@mail.gmail.com Backpatch-through: 9.4
1 parent ba1dfbe commit d76652e

File tree

9 files changed

+129
-8
lines changed

9 files changed

+129
-8
lines changed

‎doc/src/sgml/ref/create_index.sgml

+5
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,11 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ <replaceable class="parameter">name</
123123
&mdash; see <xref linkend="SQL-CREATEINDEX-CONCURRENTLY"
124124
endterm="SQL-CREATEINDEX-CONCURRENTLY-title">.
125125
</para>
126+
<para>
127+
For temporary tables, <command>CREATE INDEX</command> is always
128+
non-concurrent, as no other session can access them, and
129+
non-concurrent index creation is cheaper.
130+
</para>
126131
</listitem>
127132
</varlistentry>
128133

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

+5
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,11 @@ DROP INDEX [ CONCURRENTLY ] [ IF EXISTS ] <replaceable class="PARAMETER">name</r
5858
performed within a transaction block, but
5959
<command>DROP INDEX CONCURRENTLY</> cannot.
6060
</para>
61+
<para>
62+
For temporary tables, <command>DROP INDEX</command> is always
63+
non-concurrent, as no other session can access them, and
64+
non-concurrent index drop is cheaper.
65+
</para>
6166
</listitem>
6267
</varlistentry>
6368

‎src/backend/catalog/index.c

+9
Original file line numberDiff line numberDiff line change
@@ -1336,6 +1336,15 @@ index_drop(Oid indexId, bool concurrent)
13361336
LOCKTAG heaplocktag;
13371337
LOCKMODE lockmode;
13381338

1339+
/*
1340+
* A temporary relation uses a non-concurrent DROP. Other backends can't
1341+
* access a temporary relation, so there's no harm in grabbing a stronger
1342+
* lock (see comments in RemoveRelations), and a non-concurrent DROP is
1343+
* more efficient.
1344+
*/
1345+
Assert(get_rel_persistence(indexId) != RELPERSISTENCE_TEMP ||
1346+
!concurrent);
1347+
13391348
/*
13401349
* To drop an index safely, we must grab exclusive lock on its parent
13411350
* table. Exclusive lock on the index alone is insufficient because

‎src/backend/commands/indexcmds.c

+20-7
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,7 @@ DefineIndex(Oid relationId,
302302
bool skip_build,
303303
bool quiet)
304304
{
305+
bool concurrent;
305306
char *indexRelationName;
306307
char *accessMethodName;
307308
Oid *typeObjectId;
@@ -330,6 +331,18 @@ DefineIndex(Oid relationId,
330331
Snapshot snapshot;
331332
int i;
332333

334+
/*
335+
* Force non-concurrent build on temporary relations, even if CONCURRENTLY
336+
* was requested. Other backends can't access a temporary relation, so
337+
* there's no harm in grabbing a stronger lock, and a non-concurrent DROP
338+
* is more efficient. Do this before any use of the concurrent option is
339+
* done.
340+
*/
341+
if (stmt->concurrent && get_rel_persistence(relationId) != RELPERSISTENCE_TEMP)
342+
concurrent = true;
343+
else
344+
concurrent = false;
345+
333346
/*
334347
* count attributes in index
335348
*/
@@ -355,7 +368,7 @@ DefineIndex(Oid relationId,
355368
* relation. To avoid lock upgrade hazards, that lock should be at least
356369
* as strong as the one we take here.
357370
*/
358-
lockmode = stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock;
371+
lockmode = concurrent ? ShareUpdateExclusiveLock : ShareLock;
359372
rel = heap_open(relationId, lockmode);
360373

361374
relationId = RelationGetRelid(rel);
@@ -540,8 +553,8 @@ DefineIndex(Oid relationId,
540553
indexInfo->ii_ExclusionStrats = NULL;
541554
indexInfo->ii_Unique = stmt->unique;
542555
/* In a concurrent build, mark it not-ready-for-inserts */
543-
indexInfo->ii_ReadyForInserts = !stmt->concurrent;
544-
indexInfo->ii_Concurrent = stmt->concurrent;
556+
indexInfo->ii_ReadyForInserts = !concurrent;
557+
indexInfo->ii_Concurrent = concurrent;
545558
indexInfo->ii_BrokenHotChain = false;
546559

547560
typeObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
@@ -592,7 +605,7 @@ DefineIndex(Oid relationId,
592605
* A valid stmt->oldNode implies that we already have a built form of the
593606
* index. The caller should also decline any index build.
594607
*/
595-
Assert(!OidIsValid(stmt->oldNode) || (skip_build && !stmt->concurrent));
608+
Assert(!OidIsValid(stmt->oldNode) || (skip_build && !concurrent));
596609

597610
/*
598611
* Make the catalog entries for the index, including constraints. Then, if
@@ -606,15 +619,15 @@ DefineIndex(Oid relationId,
606619
coloptions, reloptions, stmt->primary,
607620
stmt->isconstraint, stmt->deferrable, stmt->initdeferred,
608621
allowSystemTableMods,
609-
skip_build || stmt->concurrent,
610-
stmt->concurrent, !check_rights);
622+
skip_build || concurrent,
623+
concurrent, !check_rights);
611624

612625
/* Add any requested comment */
613626
if (stmt->idxcomment != NULL)
614627
CreateComments(indexRelationId, RelationRelationId, 0,
615628
stmt->idxcomment);
616629

617-
if (!stmt->concurrent)
630+
if (!concurrent)
618631
{
619632
/* Close the heap and we're done, in the non-concurrent case */
620633
heap_close(rel, NoLock);

‎src/backend/commands/tablecmds.c

+17-1
Original file line numberDiff line numberDiff line change
@@ -784,7 +784,11 @@ RemoveRelations(DropStmt *drop)
784784
/* DROP CONCURRENTLY uses a weaker lock, and has some restrictions */
785785
if (drop->concurrent)
786786
{
787-
flags |= PERFORM_DELETION_CONCURRENTLY;
787+
/*
788+
* Note that for temporary relations this lock may get upgraded
789+
* later on, but as no other session can access a temporary
790+
* relation, this is actually fine.
791+
*/
788792
lockmode = ShareUpdateExclusiveLock;
789793
Assert(drop->removeType == OBJECT_INDEX);
790794
if (list_length(drop->objects) != 1)
@@ -875,6 +879,18 @@ RemoveRelations(DropStmt *drop)
875879
continue;
876880
}
877881

882+
/*
883+
* Decide if concurrent mode needs to be used here or not. The
884+
* relation persistence cannot be known without its OID.
885+
*/
886+
if (drop->concurrent &&
887+
get_rel_persistence(relOid) != RELPERSISTENCE_TEMP)
888+
{
889+
Assert(list_length(drop->objects) == 1 &&
890+
drop->removeType == OBJECT_INDEX);
891+
flags |= PERFORM_DELETION_CONCURRENTLY;
892+
}
893+
878894
/* OK, we're ready to delete this one */
879895
obj.classId = RelationRelationId;
880896
obj.objectId = relOid;

‎src/backend/utils/cache/lsyscache.c

+22
Original file line numberDiff line numberDiff line change
@@ -1797,6 +1797,28 @@ get_rel_tablespace(Oid relid)
17971797
return InvalidOid;
17981798
}
17991799

1800+
/*
1801+
* get_rel_persistence
1802+
*
1803+
* Returns the relpersistence associated with a given relation.
1804+
*/
1805+
char
1806+
get_rel_persistence(Oid relid)
1807+
{
1808+
HeapTuple tp;
1809+
Form_pg_class reltup;
1810+
char result;
1811+
1812+
tp = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
1813+
if (!HeapTupleIsValid(tp))
1814+
elog(ERROR, "cache lookup failed for relation %u", relid);
1815+
reltup = (Form_pg_class) GETSTRUCT(tp);
1816+
result = reltup->relpersistence;
1817+
ReleaseSysCache(tp);
1818+
1819+
return result;
1820+
}
1821+
18001822

18011823
/* ---------- TYPE CACHE ---------- */
18021824

‎src/include/utils/lsyscache.h

+1
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@ extern Oid get_rel_namespace(Oid relid);
103103
extern Oid get_rel_type_id(Oid relid);
104104
extern char get_rel_relkind(Oid relid);
105105
extern Oid get_rel_tablespace(Oid relid);
106+
extern char get_rel_persistence(Oid relid);
106107
extern bool get_typisdefined(Oid typid);
107108
extern int16 get_typlen(Oid typid);
108109
extern bool get_typbyval(Oid typid);

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

+25
Original file line numberDiff line numberDiff line change
@@ -2387,6 +2387,31 @@ Indexes:
23872387
"concur_index5" btree (f2) WHERE f1 = 'x'::text
23882388
"std_index" btree (f2)
23892389

2390+
-- Temporary tables with concurrent builds and on-commit actions
2391+
-- CONCURRENTLY used with CREATE INDEX and DROP INDEX is ignored.
2392+
-- PRESERVE ROWS, the default.
2393+
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
2394+
ON COMMIT PRESERVE ROWS;
2395+
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
2396+
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
2397+
DROP INDEX CONCURRENTLY concur_temp_ind;
2398+
DROP TABLE concur_temp;
2399+
-- ON COMMIT DROP
2400+
BEGIN;
2401+
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
2402+
ON COMMIT DROP;
2403+
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
2404+
-- Fails when running in a transaction.
2405+
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
2406+
ERROR: CREATE INDEX CONCURRENTLY cannot run inside a transaction block
2407+
COMMIT;
2408+
-- ON COMMIT DELETE ROWS
2409+
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
2410+
ON COMMIT DELETE ROWS;
2411+
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
2412+
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
2413+
DROP INDEX CONCURRENTLY concur_temp_ind;
2414+
DROP TABLE concur_temp;
23902415
--
23912416
-- Try some concurrent index drops
23922417
--

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

+25
Original file line numberDiff line numberDiff line change
@@ -756,6 +756,31 @@ VACUUM FULL concur_heap;
756756
REINDEX TABLE concur_heap;
757757
\d concur_heap
758758

759+
-- Temporary tables with concurrent builds and on-commit actions
760+
-- CONCURRENTLY used with CREATE INDEX and DROP INDEX is ignored.
761+
-- PRESERVE ROWS, the default.
762+
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
763+
ON COMMIT PRESERVE ROWS;
764+
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
765+
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
766+
DROP INDEX CONCURRENTLY concur_temp_ind;
767+
DROP TABLE concur_temp;
768+
-- ON COMMIT DROP
769+
BEGIN;
770+
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
771+
ON COMMIT DROP;
772+
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
773+
-- Fails when running in a transaction.
774+
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
775+
COMMIT;
776+
-- ON COMMIT DELETE ROWS
777+
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
778+
ON COMMIT DELETE ROWS;
779+
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
780+
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
781+
DROP INDEX CONCURRENTLY concur_temp_ind;
782+
DROP TABLE concur_temp;
783+
759784
--
760785
-- Try some concurrent index drops
761786
--

0 commit comments

Comments
 (0)