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

diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 83ee7d3f25202eec3f11904ef024d683f57d19cf..dc97ab69405f30ba3f6df76fae908c9cf9707865 100644 (file)
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -123,6 +123,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>
 

diff --git a/doc/src/sgml/ref/drop_index.sgml b/doc/src/sgml/ref/drop_index.sgml
index 6fe108ded25f6e264aaf52fffc1fad9b271d1872..4c0c48ab1ea76ce83c3143c76c81489423c19262 100644 (file)
--- a/doc/src/sgml/ref/drop_index.sgml
+++ b/doc/src/sgml/ref/drop_index.sgml
@@ -58,6 +58,11 @@ DROP INDEX [ CONCURRENTLY ] [ IF EXISTS ] <replaceable class="PARAMETER">name</r
       performed within a transaction block, but
       <command>DROP INDEX CONCURRENTLY</> 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>
 

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index f2712b94c5ec20e59bb53dbb33d4391a67612f8a..9a0f45ac3e0b1835f3c489c877abc2e86c98da2a 100644 (file)
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1374,6 +1374,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

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index e653e4f8fec11387c561c07b4dc14822a04db05a..292ba8d9b6ff83d1f0240a971d1c1519b9a32995 100644 (file)
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -315,6 +315,7 @@ DefineIndex(Oid relationId,
            bool skip_build,
            bool quiet)
 {
+   bool        concurrent;
    char       *indexRelationName;
    char       *accessMethodName;
    Oid        *typeObjectId;
@@ -345,6 +346,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
     */
@@ -370,7 +383,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);
@@ -571,8 +584,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_AmCache = NULL;
    indexInfo->ii_Context = CurrentMemoryContext;
@@ -660,7 +673,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
@@ -674,8 +687,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);
@@ -691,7 +704,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);

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 59efa323388fc4ce7e3529584a263882b333dcee..1a005760d8660408c6fa212553b4bf2a15003a85 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -966,7 +966,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)
@@ -1058,6 +1062,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;

diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 3db118177459de8b7b1f4b24d4f32a390e11b7a6..0950f99f86cd97b5fa1d27cc311c3ba67af62606 100644 (file)
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2514,6 +2514,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
 --

diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index d906d6c7050c9bd401e4defcbf6e8d9c2d2002ae..a852e7a1dbfe0be28af9d1f4ee58e08eed0c1b4b 100644 (file)
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -795,6 +795,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
 --