Re: REINDEX CONCURRENTLY unexpectedly fails
Michael Paquier <[email protected]> Wed, 15 Jan 2020 10:39:03 +0900
| Newsgroups | gmane.comp.db.postgresql.bugs |
|---|---|
| Message-ID | <[email protected]> |
--wLAMOaPNJ0fu1fTG
Content-Type: multipart/mixed; boundary="xo44VMWPx7vlQ2+2"
Content-Disposition: inline
--xo44VMWPx7vlQ2+2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Tue, Jan 14, 2020 at 11:41:11PM +0200, Heikki Linnakangas wrote:
> I'm not a fan of all those changes in RangeVarCallbackForDropRelation() to
> ensure that you get an AccessExclusiveLock to begin with. It gets pretty
> complicated, and it feels like you need to special-case temporary tables in
> dozen different places. I liked the v3 of this patch better. It's true that
> you're upgrading the ShareUpdateExclusiveLock to AccessExclusiveLock, but
> since it's a temporary table, there really should be no other backend
> holding a lock on it.
Thanks for taking the time to share your opinion. That was as well my
feeling with the peanut and the sledgehammer. I liked the peanuts,
but not the hammer part.
There are still some parts I liked about v4 (doc wording, tweaks about
the shape of RelationSupportsConcurrentIndexing and its use in
assertions, setting up the concurrent flag in RemoveRelation and use an
assert in index_drop is also cleaner), so I kept a good portion of
v4. Attached is an updated patch, v5, that removes the parts
enforcing the lock when looking at the relation OID based on its
RangeVar.
Any thoughts?
--
Michael
--xo44VMWPx7vlQ2+2
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="reindex-conc-temp-v5.patch"
Content-Transfer-Encoding: quoted-printable
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index a2890c1314..51723a8d8d 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -115,6 +115,8 @@ extern bool CompareIndexInfo(IndexInfo *info1, IndexInf=
o *info2,
=20
extern void BuildSpeculativeIndexInfo(Relation index, IndexInfo *ii);
=20
+extern bool RelationSupportsConcurrentIndexing(Oid relid);
+
extern void FormIndexDatum(IndexInfo *indexInfo,
TupleTableSlot *slot,
EState *estate,
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 3e59e647e5..4139232b51 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2016,6 +2016,13 @@ index_drop(Oid indexId, bool concurrent, bool concur=
rent_lock_mode)
LOCKTAG heaplocktag;
LOCKMODE lockmode;
=20
+ /*
+ * A relation not supporting concurrent indexing should never do
+ * a concurrent index drop or try to use a concurrent lock mode.
+ */
+ Assert(RelationSupportsConcurrentIndexing(indexId) ||
+ (!concurrent && !concurrent_lock_mode));
+
/*
* To drop an index safely, we must grab exclusive lock on its parent
* table. Exclusive lock on the index alone is insufficient because
@@ -2108,6 +2115,9 @@ index_drop(Oid indexId, bool concurrent, bool concurr=
ent_lock_mode)
*/
CacheInvalidateRelcache(userHeapRelation);
=20
+ /* Relation had better support concurrent indexing */
+ Assert(RelationSupportsConcurrentIndexing(indexId));
+
/* save lockrelid and locktag for below, then close but keep locks */
heaprelid =3D userHeapRelation->rd_lockInfo.lockRelId;
SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
@@ -2490,6 +2500,30 @@ CompareIndexInfo(IndexInfo *info1, IndexInfo *info2,
return true;
}
=20
+/*
+ * RelationSupportsConcurrentIndexing
+ *
+ * Check if a relation supports concurrent builds or not. This is used
+ * before processing CREATE INDEX, DROP INDEX or REINDEX when using
+ * CONCURRENTLY to decide if the operation is supported.
+ */
+bool
+RelationSupportsConcurrentIndexing(Oid relid)
+{
+ /*
+ * Build indexes non-concurrently for temporary relations. Such
+ * relations only work with the session assigned to them, so they are
+ * not subject to concurrent concerns, and a concurrent build would
+ * cause issues with ON COMMIT actions triggered by the transactions
+ * of the concurrent build. A non-concurrent reindex is also more
+ * efficient in this case.
+ */
+ if (get_rel_persistence(relid) =3D=3D RELPERSISTENCE_TEMP)
+ return false;
+
+ return true;
+}
+
/* ----------------
* BuildSpeculativeIndexInfo
* Add extra state to IndexInfo record
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexc=
mds.c
index 52ce02f898..d63a885638 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -485,6 +485,13 @@ DefineIndex(Oid relationId,
GUC_ACTION_SAVE, true, 0, false);
}
=20
+ /*
+ * Enforce non-concurrent build if the relation does not support this
+ * option. Do this before any use of the concurrent option is done.
+ */
+ if (!RelationSupportsConcurrentIndexing(relationId))
+ stmt->concurrent =3D false;
+
/*
* Start progress report. If we're building a partition, this was already
* done.
@@ -2347,7 +2354,7 @@ ReindexIndex(RangeVar *indexRelation, int options, bo=
ol concurrent)
persistence =3D irel->rd_rel->relpersistence;
index_close(irel, NoLock);
=20
- if (concurrent)
+ if (concurrent && RelationSupportsConcurrentIndexing(indOid))
ReindexRelationConcurrently(indOid, options);
else
reindex_index(indOid, false, persistence,
@@ -2440,7 +2447,8 @@ ReindexTable(RangeVar *relation, int options, bool co=
ncurrent)
0,
RangeVarCallbackOwnsTable, NULL);
=20
- if (concurrent)
+ if (concurrent &&
+ RelationSupportsConcurrentIndexing(heapOid))
{
result =3D ReindexRelationConcurrently(heapOid, options);
=20
@@ -2646,7 +2654,8 @@ ReindexMultipleTables(const char *objectName, Reindex=
ObjectType objectKind,
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
=20
- if (concurrent)
+ if (concurrent &&
+ RelationSupportsConcurrentIndexing(relid))
{
(void) ReindexRelationConcurrently(relid, options);
/* ReindexRelationConcurrently() does the verbose output */
@@ -2769,6 +2778,9 @@ ReindexRelationConcurrently(Oid relationOid, int opti=
ons)
/* Open relation to get its indexes */
heapRelation =3D table_open(relationOid, ShareUpdateExclusiveLock);
=20
+ /* Relation had better support concurrent indexing */
+ Assert(RelationSupportsConcurrentIndexing(relationOid));
+
/* Add all the valid indexes of relation to list */
foreach(lc, RelationGetIndexList(heapRelation))
{
@@ -2862,6 +2874,9 @@ ReindexRelationConcurrently(Oid relationOid, int opti=
ons)
/* Save the list of relation OIDs in private context */
oldcontext =3D MemoryContextSwitchTo(private_context);
=20
+ /* Relation had better support concurrent indexing */
+ Assert(RelationSupportsConcurrentIndexing(heapId));
+
/* Track the heap relation of this index for session locks */
heapRelationIds =3D list_make1_oid(heapId);
=20
@@ -2937,6 +2952,13 @@ ReindexRelationConcurrently(Oid relationOid, int opt=
ions)
heapRel =3D table_open(indexRel->rd_index->indrelid,
ShareUpdateExclusiveLock);
=20
+ /*
+ * Also check for active uses of the relation in the current
+ * transaction, including open scans and pending AFTER trigger
+ * events.
+ */
+ CheckTableNotInUse(indexRel, "REINDEX");
+
pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
RelationGetRelid(heapRel));
pgstat_progress_update_param(PROGRESS_CREATEIDX_COMMAND,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablec=
mds.c
index 2ec3fc5014..7bcd08a479 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1239,7 +1239,11 @@ RemoveRelations(DropStmt *drop)
/* DROP CONCURRENTLY uses a weaker lock, and has some restrictions */
if (drop->concurrent)
{
- flags |=3D PERFORM_DELETION_CONCURRENTLY;
+ /*
+ * Note that for temporary relations this lock may get upgraded
+ * later on, but as a session is the only able to work on its
+ * temporary relations, this is actually fine.
+ */
lockmode =3D ShareUpdateExclusiveLock;
Assert(drop->removeType =3D=3D OBJECT_INDEX);
if (list_length(drop->objects) !=3D 1)
@@ -1330,6 +1334,19 @@ RemoveRelations(DropStmt *drop)
continue;
}
=20
+ /*
+ * Decide if concurrent mode needs to be used here or not. There
+ * is no way to know if the relation supports concurrent indexing or
+ * not without knowing its OID, so this is not done beforehand.
+ */
+ if (drop->concurrent &&
+ RelationSupportsConcurrentIndexing(relOid))
+ {
+ Assert(list_length(drop->objects) =3D=3D 1 &&
+ drop->removeType =3D=3D OBJECT_INDEX);
+ flags |=3D PERFORM_DELETION_CONCURRENTLY;
+ }
+
/* OK, we're ready to delete this one */
obj.classId =3D RelationRelationId;
obj.objectId =3D relOid;
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/=
expected/create_index.out
index 6446907a65..cf1a0ca2f2 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1435,6 +1435,31 @@ Indexes:
"concur_index5" btree (f2) WHERE f1 =3D 'x'::text
"std_index" btree (f2)
=20
+-- 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
--
@@ -2418,6 +2443,55 @@ SELECT pg_get_indexdef('concur_exprs_index_pred_2'::=
regclass);
(1 row)
=20
DROP TABLE concur_exprs_tab;
+-- Temporary tables and on-commit actions, where CONCURRENTLY is ignored.
+-- ON COMMIT PRESERVE ROWS, the default.
+CREATE TEMP TABLE concur_temp_tab_1 (c1 int, c2 text)
+ ON COMMIT PRESERVE ROWS;
+INSERT INTO concur_temp_tab_1 VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX concur_temp_ind_1 ON concur_temp_tab_1(c2);
+REINDEX TABLE CONCURRENTLY concur_temp_tab_1;
+REINDEX INDEX CONCURRENTLY concur_temp_ind_1;
+-- Still fails in transaction blocks
+BEGIN;
+REINDEX INDEX CONCURRENTLY concur_temp_ind_1;
+ERROR: REINDEX CONCURRENTLY cannot run inside a transaction block
+COMMIT;
+-- ON COMMIT DELETE ROWS
+CREATE TEMP TABLE concur_temp_tab_2 (c1 int, c2 text)
+ ON COMMIT DELETE ROWS;
+CREATE INDEX concur_temp_ind_2 ON concur_temp_tab_2(c2);
+REINDEX TABLE CONCURRENTLY concur_temp_tab_2;
+REINDEX INDEX CONCURRENTLY concur_temp_ind_2;
+-- ON COMMIT DROP
+BEGIN;
+CREATE TEMP TABLE concur_temp_tab_3 (c1 int, c2 text)
+ ON COMMIT PRESERVE ROWS;
+INSERT INTO concur_temp_tab_3 VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX concur_temp_ind_3 ON concur_temp_tab_3(c2);
+-- Fails when running in a transaction
+REINDEX INDEX CONCURRENTLY concur_temp_ind_3;
+ERROR: REINDEX CONCURRENTLY cannot run inside a transaction block
+COMMIT;
+-- REINDEX SCHEMA processes all temporary relations
+CREATE TABLE reindex_temp_before AS
+SELECT oid, relname, relfilenode, relkind, reltoastrelid
+ FROM pg_class
+ WHERE relname IN ('concur_temp_ind_1', 'concur_temp_ind_2');
+SELECT pg_my_temp_schema()::regnamespace as temp_schema_name \gset
+REINDEX SCHEMA :temp_schema_name;
+SELECT b.relname,
+ b.relkind,
+ CASE WHEN a.relfilenode =3D b.relfilenode THEN 'relfilenode is unc=
hanged'
+ ELSE 'relfilenode has changed' END
+ FROM reindex_temp_before b JOIN pg_class a ON b.oid =3D a.oid
+ ORDER BY 1;
+ relname | relkind | case =20
+-------------------+---------+-------------------------
+ concur_temp_ind_1 | i | relfilenode has changed
+ concur_temp_ind_2 | i | relfilenode has changed
+(2 rows)
+
+DROP TABLE concur_temp_tab_1, concur_temp_tab_2, reindex_temp_before;
--
-- REINDEX SCHEMA
--
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/c=
reate_index.sql
index 3c0c1cdc5e..5bc9da440e 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -501,6 +501,31 @@ VACUUM FULL concur_heap;
REINDEX TABLE concur_heap;
\d concur_heap
=20
+-- 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
--
@@ -972,6 +997,48 @@ SELECT pg_get_indexdef('concur_exprs_index_pred'::regc=
lass);
SELECT pg_get_indexdef('concur_exprs_index_pred_2'::regclass);
DROP TABLE concur_exprs_tab;
=20
+-- Temporary tables and on-commit actions, where CONCURRENTLY is ignored.
+-- ON COMMIT PRESERVE ROWS, the default.
+CREATE TEMP TABLE concur_temp_tab_1 (c1 int, c2 text)
+ ON COMMIT PRESERVE ROWS;
+INSERT INTO concur_temp_tab_1 VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX concur_temp_ind_1 ON concur_temp_tab_1(c2);
+REINDEX TABLE CONCURRENTLY concur_temp_tab_1;
+REINDEX INDEX CONCURRENTLY concur_temp_ind_1;
+-- Still fails in transaction blocks
+BEGIN;
+REINDEX INDEX CONCURRENTLY concur_temp_ind_1;
+COMMIT;
+-- ON COMMIT DELETE ROWS
+CREATE TEMP TABLE concur_temp_tab_2 (c1 int, c2 text)
+ ON COMMIT DELETE ROWS;
+CREATE INDEX concur_temp_ind_2 ON concur_temp_tab_2(c2);
+REINDEX TABLE CONCURRENTLY concur_temp_tab_2;
+REINDEX INDEX CONCURRENTLY concur_temp_ind_2;
+-- ON COMMIT DROP
+BEGIN;
+CREATE TEMP TABLE concur_temp_tab_3 (c1 int, c2 text)
+ ON COMMIT PRESERVE ROWS;
+INSERT INTO concur_temp_tab_3 VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX concur_temp_ind_3 ON concur_temp_tab_3(c2);
+-- Fails when running in a transaction
+REINDEX INDEX CONCURRENTLY concur_temp_ind_3;
+COMMIT;
+-- REINDEX SCHEMA processes all temporary relations
+CREATE TABLE reindex_temp_before AS
+SELECT oid, relname, relfilenode, relkind, reltoastrelid
+ FROM pg_class
+ WHERE relname IN ('concur_temp_ind_1', 'concur_temp_ind_2');
+SELECT pg_my_temp_schema()::regnamespace as temp_schema_name \gset
+REINDEX SCHEMA :temp_schema_name;
+SELECT b.relname,
+ b.relkind,
+ CASE WHEN a.relfilenode =3D b.relfilenode THEN 'relfilenode is unc=
hanged'
+ ELSE 'relfilenode has changed' END
+ FROM reindex_temp_before b JOIN pg_class a ON b.oid =3D a.oid
+ ORDER BY 1;
+DROP TABLE concur_temp_tab_1, concur_temp_tab_2, reindex_temp_before;
+
--
-- REINDEX SCHEMA
--
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_i=
ndex.sgml
index 629a31ef79..ab362a0dc5 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -129,6 +129,11 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EX=
ISTS ] <replaceable class=3D
— see <xref linkend=3D"sql-createindex-concurrently"
endterm=3D"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>
=20
diff --git a/doc/src/sgml/ref/drop_index.sgml b/doc/src/sgml/ref/drop_index=
=2Esgml
index 2a8ca5bf68..0aedd71bd6 100644
--- 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 c=
lass=3D"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>
=20
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 5aa59d3b75..0cc19b86ee 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -166,6 +166,11 @@ REINDEX [ ( <replaceable class=3D"parameter">option</r=
eplaceable> [, ...] ) ] { IN
— see <xref linkend=3D"sql-reindex-concurrently"
endterm=3D"sql-reindex-concurrently-title"/>.
</para>
+ <para>
+ For temporary tables, <command>REINDEX</command> is always
+ non-concurrent, as no other session can access them, and
+ non-concurrent index creation is cheaper.
+ </para>
</listitem>
</varlistentry>
=20
--xo44VMWPx7vlQ2+2--
--wLAMOaPNJ0fu1fTG
Content-Type: application/pgp-signature; name="signature.asc"
-----BEGIN PGP SIGNATURE-----
iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl4ebTcACgkQnvQgOdby
QH2b1xAAhaNQnN7eS3jnO/cbOGYfxefjWglqUHMWPcN8cbia3TjyRdABi4/bp0Ll
MyhDZ5UasJOGY7TqbAKsRjHEWpc9g8Edvjx5rsvUFcA87pPOCRcn+sJTibfCtyUj
440LoZrVwqZ1/vMDrswCTNGmiTrFb+qkcgpcYXGLRMaYT1sTMUQ83sJeK8CaM+Cp
7M0ozMz1jjsgQ30ZgLVFuoAtsuOk90HpPjuFvw/2Ojj5M0yxHvQWys8f2zfBpoy9
+uaFpkH1QKduaffCSQAmPmANbRWcUtIN+LVVchN5ulYMK6leQlj1Oirbuileeboq
a1dx/AhX23/acKrnzgNA1obwamTJuPiAOfo7bbcxbJw1Ja0SuOl/Q5pUgXto+Q2p
s3a/InF33n4sEx7Kcm/I7gn02+MkjSaXKqB1Tb+m4mMNBg5q92k9cLzKxIZAekGu
WS7R/P33b0LRborElSGNmlgEQNRX2yz0GFcsht62WK9AXx3VBgSLC+nWnoXIL5IO
/YRPKrk6gWv3YevIyUpb8bv92FAnJOBy24fF/O/HXIuk59hM0/CCNHmzaPUlsHw8
GOYdxhDbn2AT8S8bmKTrc+ZK0C4le07W1Ak6YLyeOKk/OslWTNhvzs2SSa0VoYfM
4aFgtVka3NO0eneq6OOvSpbsO9v7fuWFx5ZtxGB1uF0HH22FaYE=
=GIJ6
-----END PGP SIGNATURE-----
--wLAMOaPNJ0fu1fTG--