From c39f455981770aa707f05d8e338b233e96730e47 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 22 Jan 2020 09:49:44 +0900 Subject: Fix concurrent indexing operations with temporary tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/backend/commands/indexcmds.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) (limited to 'src/backend/commands/indexcmds.c') diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index f4fd3c128e4..7313310f110 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -303,6 +303,7 @@ DefineIndex(Oid relationId, bool skip_build, bool quiet) { + bool concurrent; char *indexRelationName; char *accessMethodName; Oid *typeObjectId; @@ -332,6 +333,18 @@ DefineIndex(Oid relationId, Snapshot snapshot; 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 attributes in index */ @@ -357,7 +370,7 @@ DefineIndex(Oid relationId, * relation. To avoid lock upgrade hazards, that lock should be at least * as strong as the one we take here. */ - lockmode = stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock; + lockmode = concurrent ? ShareUpdateExclusiveLock : ShareLock; rel = heap_open(relationId, lockmode); relationId = RelationGetRelid(rel); @@ -547,8 +560,8 @@ DefineIndex(Oid relationId, indexInfo->ii_ExclusionStrats = NULL; indexInfo->ii_Unique = stmt->unique; /* In a concurrent build, mark it not-ready-for-inserts */ - indexInfo->ii_ReadyForInserts = !stmt->concurrent; - indexInfo->ii_Concurrent = stmt->concurrent; + indexInfo->ii_ReadyForInserts = !concurrent; + indexInfo->ii_Concurrent = concurrent; indexInfo->ii_BrokenHotChain = false; typeObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid)); @@ -599,7 +612,7 @@ DefineIndex(Oid relationId, * A valid stmt->oldNode implies that we already have a built form of the * 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. Then, if @@ -613,8 +626,8 @@ DefineIndex(Oid relationId, coloptions, reloptions, stmt->primary, stmt->isconstraint, stmt->deferrable, stmt->initdeferred, allowSystemTableMods, - skip_build || stmt->concurrent, - stmt->concurrent, !check_rights, + skip_build || concurrent, + concurrent, !check_rights, stmt->if_not_exists); ObjectAddressSet(address, RelationRelationId, indexRelationId); @@ -630,7 +643,7 @@ DefineIndex(Oid relationId, CreateComments(indexRelationId, RelationRelationId, 0, stmt->idxcomment); - if (!stmt->concurrent) + if (!concurrent) { /* Close the heap and we're done, in the non-concurrent case */ heap_close(rel, NoLock); -- cgit v1.2.3