]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Fix concurrent indexing operations with temporary tables
authorMichael Paquier <michael@paquier.xyz>
Wed, 22 Jan 2020 00:49:28 +0000 (09:49 +0900)
committerMichael Paquier <michael@paquier.xyz>
Wed, 22 Jan 2020 00:49:28 +0000 (09:49 +0900)
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

doc/src/sgml/ref/create_index.sgml
doc/src/sgml/ref/drop_index.sgml
src/backend/catalog/index.c
src/backend/commands/indexcmds.c
src/backend/commands/tablecmds.c
src/test/regress/expected/create_index.out
src/test/regress/sql/create_index.sql

index 3a4e71ca1cb4784b719e46d3de20eb6ea6419558..2d34e44ffff893ee8466d9dc5f6bbf4dbc4d6d1b 100644 (file)
@@ -129,6 +129,11 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
         &mdash; see <xref linkend="sql-createindex-concurrently"
         endterm="sql-createindex-concurrently-title"/>.
        </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>
      </varlistentry>
 
index 2a8ca5bf689be6e73c913dae95bfd87d19210d1e..0aedd71bd68da258a661d1aa4caaa52a05635800 100644 (file)
@@ -58,6 +58,11 @@ DROP INDEX [ CONCURRENTLY ] [ IF EXISTS ] <replaceable class="parameter">name</r
       performed within a transaction block, but
       <command>DROP INDEX CONCURRENTLY</command> cannot.
      </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>
    </varlistentry>
 
index bc20d7c750bd11621ee3fda82e1093428dfb5db9..bb7d9813273f17f438cfa130f9e88c43e3d3cfd2 100644 (file)
@@ -1493,6 +1493,15 @@ index_drop(Oid indexId, bool concurrent)
        LOCKTAG         heaplocktag;
        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
         * table.  Exclusive lock on the index alone is insufficient because
index 1faaaf625d9f207c1445dd375c4f797777455127..568a32bb4ec09e298dc4d44dd6d2cad79894cb81 100644 (file)
@@ -335,6 +335,7 @@ DefineIndex(Oid relationId,
                        bool skip_build,
                        bool quiet)
 {
+       bool            concurrent;
        char       *indexRelationName;
        char       *accessMethodName;
        Oid                *typeObjectId;
@@ -371,6 +372,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 key attributes in index
         */
@@ -413,7 +426,7 @@ DefineIndex(Oid relationId,
         * parallel workers under the control of certain particular ambuild
         * functions will need to be updated, too.
         */
-       lockmode = stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock;
+       lockmode = concurrent ? ShareUpdateExclusiveLock : ShareLock;
        rel = heap_open(relationId, lockmode);
 
        relationId = RelationGetRelid(rel);
@@ -457,6 +470,12 @@ DefineIndex(Oid relationId,
        partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
        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)
                        ereport(ERROR,
                                        (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -645,8 +664,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;
        indexInfo->ii_ParallelWorkers = 0;
        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
         * 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
@@ -825,11 +844,11 @@ DefineIndex(Oid relationId,
        flags = constr_flags = 0;
        if (stmt->isconstraint)
                flags |= INDEX_CREATE_ADD_CONSTRAINT;
-       if (skip_build || stmt->concurrent || partitioned)
+       if (skip_build || concurrent || partitioned)
                flags |= INDEX_CREATE_SKIP_BUILD;
        if (stmt->if_not_exists)
                flags |= INDEX_CREATE_IF_NOT_EXISTS;
-       if (stmt->concurrent)
+       if (concurrent)
                flags |= INDEX_CREATE_CONCURRENT;
        if (partitioned)
                flags |= INDEX_CREATE_PARTITIONED;
@@ -1107,7 +1126,7 @@ DefineIndex(Oid relationId,
                return address;
        }
 
-       if (!stmt->concurrent)
+       if (!concurrent)
        {
                /* Close the heap and we're done, in the non-concurrent case */
                heap_close(rel, NoLock);
index a120a18f029e5e99766eaca10600c74960d3da5a..38386fb9cf940eac876a625a06481f9960ddd9f2 100644 (file)
@@ -1106,7 +1106,11 @@ RemoveRelations(DropStmt *drop)
        /* DROP CONCURRENTLY uses a weaker lock, and has some restrictions */
        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;
                Assert(drop->removeType == OBJECT_INDEX);
                if (list_length(drop->objects) != 1)
@@ -1197,6 +1201,18 @@ RemoveRelations(DropStmt *drop)
                        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 */
                obj.classId = RelationRelationId;
                obj.objectId = relOid;
index e7d7c7aee6d84c24e1d7d9117ebb2f0c22021cfa..c3deccca5707f9f73d0d744bca951acef8b830ee 100644 (file)
@@ -2571,6 +2571,31 @@ Indexes:
     "concur_index5" btree (f2) WHERE f1 = 'x'::text
     "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
 --
index 283c4e6c18e4f8933763d1400f044226ed72572d..668076e2159942b72ffcfbcc52e1a68db837436b 100644 (file)
@@ -825,6 +825,31 @@ VACUUM FULL concur_heap;
 REINDEX TABLE 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
 --