mirror of
https://git.postgresql.org/git/postgresql.git
synced 2024-08-23 03:40:41 +02:00
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
This commit is contained in:
parent
c8e0e560e7
commit
5b4b07fa9a
@ -129,6 +129,11 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
|
|||||||
— see <xref linkend="sql-createindex-concurrently"
|
— see <xref linkend="sql-createindex-concurrently"
|
||||||
endterm="sql-createindex-concurrently-title"/>.
|
endterm="sql-createindex-concurrently-title"/>.
|
||||||
</para>
|
</para>
|
||||||
|
<para>
|
||||||
|
For temporary tables, <command>CREATE INDEX</command> is always
|
||||||
|
non-concurrent, as no other session can access them, and
|
||||||
|
non-concurrent index creation is cheaper.
|
||||||
|
</para>
|
||||||
</listitem>
|
</listitem>
|
||||||
</varlistentry>
|
</varlistentry>
|
||||||
|
|
||||||
|
@ -58,6 +58,11 @@ DROP INDEX [ CONCURRENTLY ] [ IF EXISTS ] <replaceable class="parameter">name</r
|
|||||||
performed within a transaction block, but
|
performed within a transaction block, but
|
||||||
<command>DROP INDEX CONCURRENTLY</command> cannot.
|
<command>DROP INDEX CONCURRENTLY</command> cannot.
|
||||||
</para>
|
</para>
|
||||||
|
<para>
|
||||||
|
For temporary tables, <command>DROP INDEX</command> is always
|
||||||
|
non-concurrent, as no other session can access them, and
|
||||||
|
non-concurrent index drop is cheaper.
|
||||||
|
</para>
|
||||||
</listitem>
|
</listitem>
|
||||||
</varlistentry>
|
</varlistentry>
|
||||||
|
|
||||||
|
@ -1493,6 +1493,15 @@ index_drop(Oid indexId, bool concurrent)
|
|||||||
LOCKTAG heaplocktag;
|
LOCKTAG heaplocktag;
|
||||||
LOCKMODE lockmode;
|
LOCKMODE lockmode;
|
||||||
|
|
||||||
|
/*
|
||||||
|
* A temporary relation uses a non-concurrent DROP. Other backends can't
|
||||||
|
* access a temporary relation, so there's no harm in grabbing a stronger
|
||||||
|
* lock (see comments in RemoveRelations), and a non-concurrent DROP is
|
||||||
|
* more efficient.
|
||||||
|
*/
|
||||||
|
Assert(get_rel_persistence(indexId) != RELPERSISTENCE_TEMP ||
|
||||||
|
!concurrent);
|
||||||
|
|
||||||
/*
|
/*
|
||||||
* To drop an index safely, we must grab exclusive lock on its parent
|
* To drop an index safely, we must grab exclusive lock on its parent
|
||||||
* table. Exclusive lock on the index alone is insufficient because
|
* table. Exclusive lock on the index alone is insufficient because
|
||||||
|
@ -335,6 +335,7 @@ DefineIndex(Oid relationId,
|
|||||||
bool skip_build,
|
bool skip_build,
|
||||||
bool quiet)
|
bool quiet)
|
||||||
{
|
{
|
||||||
|
bool concurrent;
|
||||||
char *indexRelationName;
|
char *indexRelationName;
|
||||||
char *accessMethodName;
|
char *accessMethodName;
|
||||||
Oid *typeObjectId;
|
Oid *typeObjectId;
|
||||||
@ -371,6 +372,18 @@ DefineIndex(Oid relationId,
|
|||||||
Snapshot snapshot;
|
Snapshot snapshot;
|
||||||
int i;
|
int i;
|
||||||
|
|
||||||
|
/*
|
||||||
|
* Force non-concurrent build on temporary relations, even if CONCURRENTLY
|
||||||
|
* was requested. Other backends can't access a temporary relation, so
|
||||||
|
* there's no harm in grabbing a stronger lock, and a non-concurrent DROP
|
||||||
|
* is more efficient. Do this before any use of the concurrent option is
|
||||||
|
* done.
|
||||||
|
*/
|
||||||
|
if (stmt->concurrent && get_rel_persistence(relationId) != RELPERSISTENCE_TEMP)
|
||||||
|
concurrent = true;
|
||||||
|
else
|
||||||
|
concurrent = false;
|
||||||
|
|
||||||
/*
|
/*
|
||||||
* count key attributes in index
|
* count key attributes in index
|
||||||
*/
|
*/
|
||||||
@ -413,7 +426,7 @@ DefineIndex(Oid relationId,
|
|||||||
* parallel workers under the control of certain particular ambuild
|
* parallel workers under the control of certain particular ambuild
|
||||||
* functions will need to be updated, too.
|
* functions will need to be updated, too.
|
||||||
*/
|
*/
|
||||||
lockmode = stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock;
|
lockmode = concurrent ? ShareUpdateExclusiveLock : ShareLock;
|
||||||
rel = heap_open(relationId, lockmode);
|
rel = heap_open(relationId, lockmode);
|
||||||
|
|
||||||
relationId = RelationGetRelid(rel);
|
relationId = RelationGetRelid(rel);
|
||||||
@ -457,6 +470,12 @@ DefineIndex(Oid relationId,
|
|||||||
partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
|
partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
|
||||||
if (partitioned)
|
if (partitioned)
|
||||||
{
|
{
|
||||||
|
/*
|
||||||
|
* Note: we check 'stmt->concurrent' rather than 'concurrent', so that
|
||||||
|
* the error is thrown also for temporary tables. Seems better to be
|
||||||
|
* consistent, even though we could do it on temporary table because
|
||||||
|
* we're not actually doing it concurrently.
|
||||||
|
*/
|
||||||
if (stmt->concurrent)
|
if (stmt->concurrent)
|
||||||
ereport(ERROR,
|
ereport(ERROR,
|
||||||
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
|
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
|
||||||
@ -645,8 +664,8 @@ DefineIndex(Oid relationId,
|
|||||||
indexInfo->ii_ExclusionStrats = NULL;
|
indexInfo->ii_ExclusionStrats = NULL;
|
||||||
indexInfo->ii_Unique = stmt->unique;
|
indexInfo->ii_Unique = stmt->unique;
|
||||||
/* In a concurrent build, mark it not-ready-for-inserts */
|
/* In a concurrent build, mark it not-ready-for-inserts */
|
||||||
indexInfo->ii_ReadyForInserts = !stmt->concurrent;
|
indexInfo->ii_ReadyForInserts = !concurrent;
|
||||||
indexInfo->ii_Concurrent = stmt->concurrent;
|
indexInfo->ii_Concurrent = concurrent;
|
||||||
indexInfo->ii_BrokenHotChain = false;
|
indexInfo->ii_BrokenHotChain = false;
|
||||||
indexInfo->ii_ParallelWorkers = 0;
|
indexInfo->ii_ParallelWorkers = 0;
|
||||||
indexInfo->ii_Am = accessMethodId;
|
indexInfo->ii_Am = accessMethodId;
|
||||||
@ -814,7 +833,7 @@ DefineIndex(Oid relationId,
|
|||||||
* A valid stmt->oldNode implies that we already have a built form of the
|
* A valid stmt->oldNode implies that we already have a built form of the
|
||||||
* index. The caller should also decline any index build.
|
* index. The caller should also decline any index build.
|
||||||
*/
|
*/
|
||||||
Assert(!OidIsValid(stmt->oldNode) || (skip_build && !stmt->concurrent));
|
Assert(!OidIsValid(stmt->oldNode) || (skip_build && !concurrent));
|
||||||
|
|
||||||
/*
|
/*
|
||||||
* Make the catalog entries for the index, including constraints. This
|
* Make the catalog entries for the index, including constraints. This
|
||||||
@ -825,11 +844,11 @@ DefineIndex(Oid relationId,
|
|||||||
flags = constr_flags = 0;
|
flags = constr_flags = 0;
|
||||||
if (stmt->isconstraint)
|
if (stmt->isconstraint)
|
||||||
flags |= INDEX_CREATE_ADD_CONSTRAINT;
|
flags |= INDEX_CREATE_ADD_CONSTRAINT;
|
||||||
if (skip_build || stmt->concurrent || partitioned)
|
if (skip_build || concurrent || partitioned)
|
||||||
flags |= INDEX_CREATE_SKIP_BUILD;
|
flags |= INDEX_CREATE_SKIP_BUILD;
|
||||||
if (stmt->if_not_exists)
|
if (stmt->if_not_exists)
|
||||||
flags |= INDEX_CREATE_IF_NOT_EXISTS;
|
flags |= INDEX_CREATE_IF_NOT_EXISTS;
|
||||||
if (stmt->concurrent)
|
if (concurrent)
|
||||||
flags |= INDEX_CREATE_CONCURRENT;
|
flags |= INDEX_CREATE_CONCURRENT;
|
||||||
if (partitioned)
|
if (partitioned)
|
||||||
flags |= INDEX_CREATE_PARTITIONED;
|
flags |= INDEX_CREATE_PARTITIONED;
|
||||||
@ -1107,7 +1126,7 @@ DefineIndex(Oid relationId,
|
|||||||
return address;
|
return address;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (!stmt->concurrent)
|
if (!concurrent)
|
||||||
{
|
{
|
||||||
/* Close the heap and we're done, in the non-concurrent case */
|
/* Close the heap and we're done, in the non-concurrent case */
|
||||||
heap_close(rel, NoLock);
|
heap_close(rel, NoLock);
|
||||||
|
@ -1106,7 +1106,11 @@ RemoveRelations(DropStmt *drop)
|
|||||||
/* DROP CONCURRENTLY uses a weaker lock, and has some restrictions */
|
/* DROP CONCURRENTLY uses a weaker lock, and has some restrictions */
|
||||||
if (drop->concurrent)
|
if (drop->concurrent)
|
||||||
{
|
{
|
||||||
flags |= PERFORM_DELETION_CONCURRENTLY;
|
/*
|
||||||
|
* Note that for temporary relations this lock may get upgraded
|
||||||
|
* later on, but as no other session can access a temporary
|
||||||
|
* relation, this is actually fine.
|
||||||
|
*/
|
||||||
lockmode = ShareUpdateExclusiveLock;
|
lockmode = ShareUpdateExclusiveLock;
|
||||||
Assert(drop->removeType == OBJECT_INDEX);
|
Assert(drop->removeType == OBJECT_INDEX);
|
||||||
if (list_length(drop->objects) != 1)
|
if (list_length(drop->objects) != 1)
|
||||||
@ -1197,6 +1201,18 @@ RemoveRelations(DropStmt *drop)
|
|||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/*
|
||||||
|
* Decide if concurrent mode needs to be used here or not. The
|
||||||
|
* relation persistence cannot be known without its OID.
|
||||||
|
*/
|
||||||
|
if (drop->concurrent &&
|
||||||
|
get_rel_persistence(relOid) != RELPERSISTENCE_TEMP)
|
||||||
|
{
|
||||||
|
Assert(list_length(drop->objects) == 1 &&
|
||||||
|
drop->removeType == OBJECT_INDEX);
|
||||||
|
flags |= PERFORM_DELETION_CONCURRENTLY;
|
||||||
|
}
|
||||||
|
|
||||||
/* OK, we're ready to delete this one */
|
/* OK, we're ready to delete this one */
|
||||||
obj.classId = RelationRelationId;
|
obj.classId = RelationRelationId;
|
||||||
obj.objectId = relOid;
|
obj.objectId = relOid;
|
||||||
|
@ -2571,6 +2571,31 @@ Indexes:
|
|||||||
"concur_index5" btree (f2) WHERE f1 = 'x'::text
|
"concur_index5" btree (f2) WHERE f1 = 'x'::text
|
||||||
"std_index" btree (f2)
|
"std_index" btree (f2)
|
||||||
|
|
||||||
|
-- Temporary tables with concurrent builds and on-commit actions
|
||||||
|
-- CONCURRENTLY used with CREATE INDEX and DROP INDEX is ignored.
|
||||||
|
-- PRESERVE ROWS, the default.
|
||||||
|
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
|
||||||
|
ON COMMIT PRESERVE ROWS;
|
||||||
|
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
|
||||||
|
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
|
||||||
|
DROP INDEX CONCURRENTLY concur_temp_ind;
|
||||||
|
DROP TABLE concur_temp;
|
||||||
|
-- ON COMMIT DROP
|
||||||
|
BEGIN;
|
||||||
|
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
|
||||||
|
ON COMMIT DROP;
|
||||||
|
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
|
||||||
|
-- Fails when running in a transaction.
|
||||||
|
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
|
||||||
|
ERROR: CREATE INDEX CONCURRENTLY cannot run inside a transaction block
|
||||||
|
COMMIT;
|
||||||
|
-- ON COMMIT DELETE ROWS
|
||||||
|
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
|
||||||
|
ON COMMIT DELETE ROWS;
|
||||||
|
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
|
||||||
|
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
|
||||||
|
DROP INDEX CONCURRENTLY concur_temp_ind;
|
||||||
|
DROP TABLE concur_temp;
|
||||||
--
|
--
|
||||||
-- Try some concurrent index drops
|
-- Try some concurrent index drops
|
||||||
--
|
--
|
||||||
|
@ -825,6 +825,31 @@ VACUUM FULL concur_heap;
|
|||||||
REINDEX TABLE concur_heap;
|
REINDEX TABLE concur_heap;
|
||||||
\d concur_heap
|
\d concur_heap
|
||||||
|
|
||||||
|
-- Temporary tables with concurrent builds and on-commit actions
|
||||||
|
-- CONCURRENTLY used with CREATE INDEX and DROP INDEX is ignored.
|
||||||
|
-- PRESERVE ROWS, the default.
|
||||||
|
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
|
||||||
|
ON COMMIT PRESERVE ROWS;
|
||||||
|
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
|
||||||
|
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
|
||||||
|
DROP INDEX CONCURRENTLY concur_temp_ind;
|
||||||
|
DROP TABLE concur_temp;
|
||||||
|
-- ON COMMIT DROP
|
||||||
|
BEGIN;
|
||||||
|
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
|
||||||
|
ON COMMIT DROP;
|
||||||
|
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
|
||||||
|
-- Fails when running in a transaction.
|
||||||
|
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
|
||||||
|
COMMIT;
|
||||||
|
-- ON COMMIT DELETE ROWS
|
||||||
|
CREATE TEMP TABLE concur_temp (f1 int, f2 text)
|
||||||
|
ON COMMIT DELETE ROWS;
|
||||||
|
INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
|
||||||
|
CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
|
||||||
|
DROP INDEX CONCURRENTLY concur_temp_ind;
|
||||||
|
DROP TABLE concur_temp;
|
||||||
|
|
||||||
--
|
--
|
||||||
-- Try some concurrent index drops
|
-- Try some concurrent index drops
|
||||||
--
|
--
|
||||||
|
Loading…
Reference in New Issue
Block a user