Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit a904abe

Browse filesBrowse 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 9b9c5f2 commit a904abe
Copy full SHA for a904abe

File tree

Expand file treeCollapse file tree

8 files changed

+235
-13
lines changed
Filter options
Expand file treeCollapse file tree

8 files changed

+235
-13
lines changed

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

Copy file name to clipboardExpand all lines: doc/src/sgml/ref/create_index.sgml
+5Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,11 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
129129
&mdash; see <xref linkend="sql-createindex-concurrently"
130130
endterm="sql-createindex-concurrently-title"/>.
131131
</para>
132+
<para>
133+
For temporary tables, <command>CREATE INDEX</command> is always
134+
non-concurrent, as no other session can access them, and
135+
non-concurrent index creation is cheaper.
136+
</para>
132137
</listitem>
133138
</varlistentry>
134139

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

Copy file name to clipboardExpand all lines: doc/src/sgml/ref/drop_index.sgml
+5Lines changed: 5 additions & 0 deletions
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</command> 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

‎doc/src/sgml/ref/reindex.sgml

Copy file name to clipboardExpand all lines: doc/src/sgml/ref/reindex.sgml
+5Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,11 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
166166
&mdash; see <xref linkend="sql-reindex-concurrently"
167167
endterm="sql-reindex-concurrently-title"/>.
168168
</para>
169+
<para>
170+
For temporary tables, <command>REINDEX</command> is always
171+
non-concurrent, as no other session can access them, and
172+
non-concurrent reindex is cheaper.
173+
</para>
169174
</listitem>
170175
</varlistentry>
171176

‎src/backend/catalog/index.c

Copy file name to clipboardExpand all lines: src/backend/catalog/index.c
+9Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2016,6 +2016,15 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode)
20162016
LOCKTAG heaplocktag;
20172017
LOCKMODE lockmode;
20182018

2019+
/*
2020+
* A temporary relation uses a non-concurrent DROP. Other backends can't
2021+
* access a temporary relation, so there's no harm in grabbing a stronger
2022+
* lock (see comments in RemoveRelations), and a non-concurrent DROP is
2023+
* more efficient.
2024+
*/
2025+
Assert(get_rel_persistence(indexId) != RELPERSISTENCE_TEMP ||
2026+
(!concurrent && !concurrent_lock_mode));
2027+
20192028
/*
20202029
* To drop an index safely, we must grab exclusive lock on its parent
20212030
* table. Exclusive lock on the index alone is insufficient because

‎src/backend/commands/indexcmds.c

Copy file name to clipboardExpand all lines: src/backend/commands/indexcmds.c
+53-12Lines changed: 53 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,7 @@ DefineIndex(Oid relationId,
438438
bool skip_build,
439439
bool quiet)
440440
{
441+
bool concurrent;
441442
char *indexRelationName;
442443
char *accessMethodName;
443444
Oid *typeObjectId;
@@ -485,6 +486,18 @@ DefineIndex(Oid relationId,
485486
GUC_ACTION_SAVE, true, 0, false);
486487
}
487488

489+
/*
490+
* Force non-concurrent build on temporary relations, even if CONCURRENTLY
491+
* was requested. Other backends can't access a temporary relation, so
492+
* there's no harm in grabbing a stronger lock, and a non-concurrent DROP
493+
* is more efficient. Do this before any use of the concurrent option is
494+
* done.
495+
*/
496+
if (stmt->concurrent && get_rel_persistence(relationId) != RELPERSISTENCE_TEMP)
497+
concurrent = true;
498+
else
499+
concurrent = false;
500+
488501
/*
489502
* Start progress report. If we're building a partition, this was already
490503
* done.
@@ -494,7 +507,7 @@ DefineIndex(Oid relationId,
494507
pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
495508
relationId);
496509
pgstat_progress_update_param(PROGRESS_CREATEIDX_COMMAND,
497-
stmt->concurrent ?
510+
concurrent ?
498511
PROGRESS_CREATEIDX_COMMAND_CREATE_CONCURRENTLY :
499512
PROGRESS_CREATEIDX_COMMAND_CREATE);
500513
}
@@ -547,7 +560,7 @@ DefineIndex(Oid relationId,
547560
* parallel workers under the control of certain particular ambuild
548561
* functions will need to be updated, too.
549562
*/
550-
lockmode = stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock;
563+
lockmode = concurrent ? ShareUpdateExclusiveLock : ShareLock;
551564
rel = table_open(relationId, lockmode);
552565

553566
namespaceId = RelationGetNamespace(rel);
@@ -590,6 +603,12 @@ DefineIndex(Oid relationId,
590603
partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
591604
if (partitioned)
592605
{
606+
/*
607+
* Note: we check 'stmt->concurrent' rather than 'concurrent', so that
608+
* the error is thrown also for temporary tables. Seems better to be
609+
* consistent, even though we could do it on temporary table because
610+
* we're not actually doing it concurrently.
611+
*/
593612
if (stmt->concurrent)
594613
ereport(ERROR,
595614
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -781,8 +800,8 @@ DefineIndex(Oid relationId,
781800
NIL, /* expressions, NIL for now */
782801
make_ands_implicit((Expr *) stmt->whereClause),
783802
stmt->unique,
784-
!stmt->concurrent,
785-
stmt->concurrent);
803+
!concurrent,
804+
concurrent);
786805

787806
typeObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
788807
collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
@@ -944,7 +963,7 @@ DefineIndex(Oid relationId,
944963
* A valid stmt->oldNode implies that we already have a built form of the
945964
* index. The caller should also decline any index build.
946965
*/
947-
Assert(!OidIsValid(stmt->oldNode) || (skip_build && !stmt->concurrent));
966+
Assert(!OidIsValid(stmt->oldNode) || (skip_build && !concurrent));
948967

949968
/*
950969
* Make the catalog entries for the index, including constraints. This
@@ -955,11 +974,11 @@ DefineIndex(Oid relationId,
955974
flags = constr_flags = 0;
956975
if (stmt->isconstraint)
957976
flags |= INDEX_CREATE_ADD_CONSTRAINT;
958-
if (skip_build || stmt->concurrent || partitioned)
977+
if (skip_build || concurrent || partitioned)
959978
flags |= INDEX_CREATE_SKIP_BUILD;
960979
if (stmt->if_not_exists)
961980
flags |= INDEX_CREATE_IF_NOT_EXISTS;
962-
if (stmt->concurrent)
981+
if (concurrent)
963982
flags |= INDEX_CREATE_CONCURRENT;
964983
if (partitioned)
965984
flags |= INDEX_CREATE_PARTITIONED;
@@ -1253,7 +1272,7 @@ DefineIndex(Oid relationId,
12531272
return address;
12541273
}
12551274

1256-
if (!stmt->concurrent)
1275+
if (!concurrent)
12571276
{
12581277
/* Close the heap and we're done, in the non-concurrent case */
12591278
table_close(rel, NoLock);
@@ -2323,6 +2342,11 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
23232342
* Find and lock index, and check permissions on table; use callback to
23242343
* obtain lock on table first, to avoid deadlock hazard. The lock level
23252344
* used here must match the index lock obtained in reindex_index().
2345+
*
2346+
* If it's a temporary index, we will perform a non-concurrent reindex,
2347+
* even if CONCURRENTLY was requested. In that case, reindex_index() will
2348+
* upgrade the lock, but that's OK, because other sessions can't hold
2349+
* locks on our temporary table.
23262350
*/
23272351
state.concurrent = concurrent;
23282352
state.locked_table_oid = InvalidOid;
@@ -2347,7 +2371,7 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
23472371
persistence = irel->rd_rel->relpersistence;
23482372
index_close(irel, NoLock);
23492373

2350-
if (concurrent)
2374+
if (concurrent && persistence != RELPERSISTENCE_TEMP)
23512375
ReindexRelationConcurrently(indOid, options);
23522376
else
23532377
reindex_index(indOid, false, persistence,
@@ -2434,13 +2458,20 @@ ReindexTable(RangeVar *relation, int options, bool concurrent)
24342458
Oid heapOid;
24352459
bool result;
24362460

2437-
/* The lock level used here should match reindex_relation(). */
2461+
/*
2462+
* The lock level used here should match reindex_relation().
2463+
*
2464+
* If it's a temporary table, we will perform a non-concurrent reindex,
2465+
* even if CONCURRENTLY was requested. In that case, reindex_relation()
2466+
* will upgrade the lock, but that's OK, because other sessions can't hold
2467+
* locks on our temporary table.
2468+
*/
24382469
heapOid = RangeVarGetRelidExtended(relation,
24392470
concurrent ? ShareUpdateExclusiveLock : ShareLock,
24402471
0,
24412472
RangeVarCallbackOwnsTable, NULL);
24422473

2443-
if (concurrent)
2474+
if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
24442475
{
24452476
result = ReindexRelationConcurrently(heapOid, options);
24462477

@@ -2646,7 +2677,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
26462677
/* functions in indexes may want a snapshot set */
26472678
PushActiveSnapshot(GetTransactionSnapshot());
26482679

2649-
if (concurrent)
2680+
if (concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP)
26502681
{
26512682
(void) ReindexRelationConcurrently(relid, options);
26522683
/* ReindexRelationConcurrently() does the verbose output */
@@ -2694,6 +2725,12 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
26942725
*
26952726
* Returns true if any indexes have been rebuilt (including toast table's
26962727
* indexes, when relevant), otherwise returns false.
2728+
*
2729+
* NOTE: This cannot be used on temporary relations. A concurrent build would
2730+
* cause issues with ON COMMIT actions triggered by the transactions of the
2731+
* concurrent build. Temporary relations are not subject to concurrent
2732+
* concerns, so there's no need for the more complicated concurrent build,
2733+
* anyway, and a non-concurrent reindex is more efficient.
26972734
*/
26982735
static bool
26992736
ReindexRelationConcurrently(Oid relationOid, int options)
@@ -2937,6 +2974,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
29372974
heapRel = table_open(indexRel->rd_index->indrelid,
29382975
ShareUpdateExclusiveLock);
29392976

2977+
/* This function shouldn't be called for temporary relations. */
2978+
if (indexRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
2979+
elog(ERROR, "cannot reindex a temporary table concurrently");
2980+
29402981
pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
29412982
RelationGetRelid(heapRel));
29422983
pgstat_progress_update_param(PROGRESS_CREATEIDX_COMMAND,

‎src/backend/commands/tablecmds.c

Copy file name to clipboardExpand all lines: src/backend/commands/tablecmds.c
+17-1Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1260,7 +1260,11 @@ RemoveRelations(DropStmt *drop)
12601260
/* DROP CONCURRENTLY uses a weaker lock, and has some restrictions */
12611261
if (drop->concurrent)
12621262
{
1263-
flags |= PERFORM_DELETION_CONCURRENTLY;
1263+
/*
1264+
* Note that for temporary relations this lock may get upgraded
1265+
* later on, but as no other session can access a temporary
1266+
* relation, this is actually fine.
1267+
*/
12641268
lockmode = ShareUpdateExclusiveLock;
12651269
Assert(drop->removeType == OBJECT_INDEX);
12661270
if (list_length(drop->objects) != 1)
@@ -1351,6 +1355,18 @@ RemoveRelations(DropStmt *drop)
13511355
continue;
13521356
}
13531357

1358+
/*
1359+
* Decide if concurrent mode needs to be used here or not. The
1360+
* relation persistence cannot be known without its OID.
1361+
*/
1362+
if (drop->concurrent &&
1363+
get_rel_persistence(relOid) != RELPERSISTENCE_TEMP)
1364+
{
1365+
Assert(list_length(drop->objects) == 1 &&
1366+
drop->removeType == OBJECT_INDEX);
1367+
flags |= PERFORM_DELETION_CONCURRENTLY;
1368+
}
1369+
13541370
/* OK, we're ready to delete this one */
13551371
obj.classId = RelationRelationId;
13561372
obj.objectId = relOid;

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

Copy file name to clipboardExpand all lines: src/test/regress/expected/create_index.out
+74Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1435,6 +1435,31 @@ Indexes:
14351435
"concur_index5" btree (f2) WHERE f1 = 'x'::text
14361436
"std_index" btree (f2)
14371437

1438+
-- Temporary tables with concurrent builds and on-commit actions
1439+
-- CONCURRENTLY used with CREATE INDEX and DROP INDEX is ignored.
1440+
-- PRESERVE ROWS, the default.
1441+
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
1442+
ON COMMIT PRESERVE ROWS;
1443+
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
1444+
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
1445+
DROP INDEX CONCURRENTLY concur_temp_ind;
1446+
DROP TABLE concur_temp;
1447+
-- ON COMMIT DROP
1448+
BEGIN;
1449+
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
1450+
ON COMMIT DROP;
1451+
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
1452+
-- Fails when running in a transaction.
1453+
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
1454+
ERROR: CREATE INDEX CONCURRENTLY cannot run inside a transaction block
1455+
COMMIT;
1456+
-- ON COMMIT DELETE ROWS
1457+
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
1458+
ON COMMIT DELETE ROWS;
1459+
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
1460+
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
1461+
DROP INDEX CONCURRENTLY concur_temp_ind;
1462+
DROP TABLE concur_temp;
14381463
--
14391464
-- Try some concurrent index drops
14401465
--
@@ -2418,6 +2443,55 @@ SELECT pg_get_indexdef('concur_exprs_index_pred_2'::regclass);
24182443
(1 row)
24192444

24202445
DROP TABLE concur_exprs_tab;
2446+
-- Temporary tables and on-commit actions, where CONCURRENTLY is ignored.
2447+
-- ON COMMIT PRESERVE ROWS, the default.
2448+
CREATE TEMP TABLE concur_temp_tab_1 (c1 int, c2 text)
2449+
ON COMMIT PRESERVE ROWS;
2450+
INSERT INTO concur_temp_tab_1 VALUES (1, 'foo'), (2, 'bar');
2451+
CREATE INDEX concur_temp_ind_1 ON concur_temp_tab_1(c2);
2452+
REINDEX TABLE CONCURRENTLY concur_temp_tab_1;
2453+
REINDEX INDEX CONCURRENTLY concur_temp_ind_1;
2454+
-- Still fails in transaction blocks
2455+
BEGIN;
2456+
REINDEX INDEX CONCURRENTLY concur_temp_ind_1;
2457+
ERROR: REINDEX CONCURRENTLY cannot run inside a transaction block
2458+
COMMIT;
2459+
-- ON COMMIT DELETE ROWS
2460+
CREATE TEMP TABLE concur_temp_tab_2 (c1 int, c2 text)
2461+
ON COMMIT DELETE ROWS;
2462+
CREATE INDEX concur_temp_ind_2 ON concur_temp_tab_2(c2);
2463+
REINDEX TABLE CONCURRENTLY concur_temp_tab_2;
2464+
REINDEX INDEX CONCURRENTLY concur_temp_ind_2;
2465+
-- ON COMMIT DROP
2466+
BEGIN;
2467+
CREATE TEMP TABLE concur_temp_tab_3 (c1 int, c2 text)
2468+
ON COMMIT PRESERVE ROWS;
2469+
INSERT INTO concur_temp_tab_3 VALUES (1, 'foo'), (2, 'bar');
2470+
CREATE INDEX concur_temp_ind_3 ON concur_temp_tab_3(c2);
2471+
-- Fails when running in a transaction
2472+
REINDEX INDEX CONCURRENTLY concur_temp_ind_3;
2473+
ERROR: REINDEX CONCURRENTLY cannot run inside a transaction block
2474+
COMMIT;
2475+
-- REINDEX SCHEMA processes all temporary relations
2476+
CREATE TABLE reindex_temp_before AS
2477+
SELECT oid, relname, relfilenode, relkind, reltoastrelid
2478+
FROM pg_class
2479+
WHERE relname IN ('concur_temp_ind_1', 'concur_temp_ind_2');
2480+
SELECT pg_my_temp_schema()::regnamespace as temp_schema_name \gset
2481+
REINDEX SCHEMA CONCURRENTLY :temp_schema_name;
2482+
SELECT b.relname,
2483+
b.relkind,
2484+
CASE WHEN a.relfilenode = b.relfilenode THEN 'relfilenode is unchanged'
2485+
ELSE 'relfilenode has changed' END
2486+
FROM reindex_temp_before b JOIN pg_class a ON b.oid = a.oid
2487+
ORDER BY 1;
2488+
relname | relkind | case
2489+
-------------------+---------+-------------------------
2490+
concur_temp_ind_1 | i | relfilenode has changed
2491+
concur_temp_ind_2 | i | relfilenode has changed
2492+
(2 rows)
2493+
2494+
DROP TABLE concur_temp_tab_1, concur_temp_tab_2, reindex_temp_before;
24212495
--
24222496
-- REINDEX SCHEMA
24232497
--

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.