REINDEX CONCURRENTLY unexpectedly fails

On Wed, Nov 13, 2019 at 9:30 AM Manuel Rigger <rigger.manuel@gmail.com>
wrote:

Hi everyone,

On the latest trunk version, I get an error "index "t0_pkey_ccnew"
already contains data" when using REINDEX CONCURRENTLY:

CREATE TEMP TABLE t0(c1 INT PRIMARY KEY) ON COMMIT DELETE ROWS;
REINDEX TABLE CONCURRENTLY t0; -- unexpected: ERROR: index
"t0_pkey_ccnew" already contains data

Is this expected? I think I did not observe this error on earlier
PostgreSQL versions.

For what it is worth, I get the samer error in 12.0. And the command
doesn't exist in 11.

The "ON COMMIT DELETE ROWS" is necessary to repodcue the problem, but the
index doesn't need to be primary key.

Cheers,

Jeff

Hi,

On 2019-11-13 15:29:53 +0100, Manuel Rigger wrote:

On the latest trunk version, I get an error "index "t0_pkey_ccnew"
already contains data" when using REINDEX CONCURRENTLY:

CREATE TEMP TABLE t0(c1 INT PRIMARY KEY) ON COMMIT DELETE ROWS;
REINDEX TABLE CONCURRENTLY t0; -- unexpected: ERROR: index
"t0_pkey_ccnew" already contains data

Is this expected? I think I did not observe this error on earlier
PostgreSQL versions.

That seems pretty clearly a bug.

The problem is that the CONCURRENTLY code executes the ON COMMIT action
during CIC's internal transactions. Which then pretty completely breaks
the REINDEX operation. I think there's also a clear lack of error
checking about the index still being the correct one in the CIC code
(not recent), and I think we also need more error checking for the
truncate code (something CheckTableNotInUse() like).

The trace:
#0 index_build (heapRelation=0x7f006d49b998, indexRelation=0x7f006d499b80, indexInfo=0x55a46121b858, isreindex=true, parallel=false)
at /home/andres/src/postgresql/src/backend/catalog/index.c:2758
#1 0x000055a45fd43853 in RelationTruncateIndexes (heapRelation=0x7f006d49b998) at /home/andres/src/postgresql/src/backend/catalog/heap.c:3161
#2 0x000055a45fd43b86 in heap_truncate_one_rel (rel=0x7f006d49b998) at /home/andres/src/postgresql/src/backend/catalog/heap.c:3234
#3 0x000055a45fd43a6d in heap_truncate (relids=0x55a46121b820) at /home/andres/src/postgresql/src/backend/catalog/heap.c:3202
#4 0x000055a45ff337cb in PreCommit_on_commit_actions () at /home/andres/src/postgresql/src/backend/commands/tablecmds.c:14652
#5 0x000055a45fcd7258 in CommitTransaction () at /home/andres/src/postgresql/src/backend/access/transam/xact.c:2110
#6 0x000055a45fcd8e80 in CommitTransactionCommand () at /home/andres/src/postgresql/src/backend/access/transam/xact.c:2923
#7 0x000055a45fecb790 in ReindexRelationConcurrently (relationOid=16409, options=0) at /home/andres/src/postgresql/src/backend/commands/indexcmds.c:3035
#8 0x000055a45fec9380 in ReindexTable (relation=0x55a461084858, options=0, concurrent=true)
at /home/andres/src/postgresql/src/backend/commands/indexcmds.c:2447

*ponders*

This probably is triggerable before v12 as well. Not with REINDEX
CONCURRENTLY, but with CREATE INDEX CONCURRENTLY.

Indeed:

postgres[7782][1]=# CREATE TEMP TABLE t0(c1 INT PRIMARY KEY) ON COMMIT DELETE ROWS;
CREATE TABLE

postgres[7782][1]=# CREATE INDEX CONCURRENTLY t0_c1 ON t0(c1);
ERROR: XX000: index "t0_c1" already contains data
LOCATION: btbuild, nbtsort.c:321

postgres[7782][1]=# SELECT version();
┌──────────────────────────────────────────────────────────────────────────────────────────────────┐
│ version │
├──────────────────────────────────────────────────────────────────────────────────────────────────┤
│ PostgreSQL 11.5 on x86_64-pc-linux-gnu, compiled by gcc (Debian 9.2.1-15) 9.2.1 20191027, 64-bit │
└──────────────────────────────────────────────────────────────────────────────────────────────────┘
(1 row)

I think this quite possibly has been broken since CIC's introduction.

It think we really ought to just refuse CIC (and thereby REINDEX
CONCURRENTLY) for ON COMMIT DELETE/DROP temp tables. Given that CIC
internally uses transactions, it makes no sense to use CIC on such a
table.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2019-11-13 15:29:53 +0100, Manuel Rigger wrote:

On the latest trunk version, I get an error "index "t0_pkey_ccnew"
already contains data" when using REINDEX CONCURRENTLY:

CREATE TEMP TABLE t0(c1 INT PRIMARY KEY) ON COMMIT DELETE ROWS;
REINDEX TABLE CONCURRENTLY t0; -- unexpected: ERROR: index
"t0_pkey_ccnew" already contains data

It think we really ought to just refuse CIC (and thereby REINDEX
CONCURRENTLY) for ON COMMIT DELETE/DROP temp tables. Given that CIC
internally uses transactions, it makes no sense to use CIC on such a
table.

It's not real clear why there would be any point in (RE)INDEX
CONCURRENTLY on a temp table anyway, since no other session could
be using it. +1 for just erroring out, rather than working
hard to support such a case.

regards, tom lane

Hi,

On 2019-11-13 10:59:08 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2019-11-13 15:29:53 +0100, Manuel Rigger wrote:

On the latest trunk version, I get an error "index "t0_pkey_ccnew"
already contains data" when using REINDEX CONCURRENTLY:

CREATE TEMP TABLE t0(c1 INT PRIMARY KEY) ON COMMIT DELETE ROWS;
REINDEX TABLE CONCURRENTLY t0; -- unexpected: ERROR: index
"t0_pkey_ccnew" already contains data

It think we really ought to just refuse CIC (and thereby REINDEX
CONCURRENTLY) for ON COMMIT DELETE/DROP temp tables. Given that CIC
internally uses transactions, it makes no sense to use CIC on such a
table.

It's not real clear why there would be any point in (RE)INDEX
CONCURRENTLY on a temp table anyway, since no other session could
be using it.

Right.

I guess it's not necessarily always clear in all contexts that one is
dealing with a temp table, rather than a normal table.

+1 for just erroring out, rather than working hard to support such a
case.

I wonder if we instead ought to just ignore the CONCURRENTLY when
targetting a temp table? That'd be a correct optimization for temp
tables, and would fix the issue at hand...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2019-11-13 10:59:08 -0500, Tom Lane wrote:

It's not real clear why there would be any point in (RE)INDEX
CONCURRENTLY on a temp table anyway, since no other session could
be using it.

I guess it's not necessarily always clear in all contexts that one is
dealing with a temp table, rather than a normal table.

That's a good point.

I wonder if we instead ought to just ignore the CONCURRENTLY when
targetting a temp table? That'd be a correct optimization for temp
tables, and would fix the issue at hand...

Oh, I like that idea. Keeps applications from having to think
about this.

regards, tom lane

On Wed, Nov 13, 2019 at 11:45:34AM -0500, Tom Lane wrote:

Oh, I like that idea. Keeps applications from having to think
about this.

That's interesting, but I would be on the side of just generating an
error in this case thinking about potential future features like
global temporary tables, and because it could always be relaxed in the
future.

I am actually wondering if we don't have more problems with other
utility commands which spawn multiple transactions...

Any extra opinion?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Wed, Nov 13, 2019 at 11:45:34AM -0500, Tom Lane wrote:

Oh, I like that idea. Keeps applications from having to think
about this.

That's interesting, but I would be on the side of just generating an
error in this case thinking about potential future features like
global temporary tables, and because it could always be relaxed in the
future.

I don't find that very convincing. If there's a reason to throw
error for global temporary tables, let's do it for that case,
but that's no reason to make the user-visible behavior overcomplex
for other cases. It might well be that we can handle global temp
tables the same way anyway (ie, just do a not-CONCURRENTLY reindex
on the session's private instance of the table).

I am actually wondering if we don't have more problems with other
utility commands which spawn multiple transactions...

Indeed, but there aren't many of those...

regards, tom lane

On Thu, Nov 14, 2019 at 12:53:53PM -0500, Tom Lane wrote:

I don't find that very convincing. If there's a reason to throw
error for global temporary tables, let's do it for that case,
but that's no reason to make the user-visible behavior overcomplex
for other cases. It might well be that we can handle global temp
tables the same way anyway (ie, just do a not-CONCURRENTLY reindex
on the session's private instance of the table).

Well, there is also the argument of consistency. What should we do if
trying to reindex concurrently a database or a schema and that the
database or the schema include both temporary and non-temporary
tables? We cannot ignore CONCURRENTLY in this case for all the
relations if there is at least one temporary table. It could be as
well surprising to skip only a portion of temporary relations (these
with on-commit actions and issue a WARNING for each one of them, still
that would be more consistent with the treatment we do for system
catalogs in ReindexMultipleTables().

An extra solution I can think of is to not skip temporary tables with
on-commit actions, but just fallback to the non-concurrent path in
ReindexMultipleTables when reindexing each relation for any temporary
tables processed (all of them, and not just these with on-commit
actions actually).

Thoughts?
--
Michael

Hi,

On 2019-11-15 11:45:12 +0900, Michael Paquier wrote:

On Thu, Nov 14, 2019 at 12:53:53PM -0500, Tom Lane wrote:

I don't find that very convincing. If there's a reason to throw
error for global temporary tables, let's do it for that case,
but that's no reason to make the user-visible behavior overcomplex
for other cases. It might well be that we can handle global temp
tables the same way anyway (ie, just do a not-CONCURRENTLY reindex
on the session's private instance of the table).

Well, there is also the argument of consistency. What should we do if
trying to reindex concurrently a database or a schema and that the
database or the schema include both temporary and non-temporary
tables? We cannot ignore CONCURRENTLY in this case for all the
relations if there is at least one temporary table. It could be as
well surprising to skip only a portion of temporary relations (these
with on-commit actions and issue a WARNING for each one of them, still
that would be more consistent with the treatment we do for system
catalogs in ReindexMultipleTables().

Who said anything about skipping? What I was talking about was to
execute a non-concurrent truncate for temp tables? Given that there
never may be any relevant concurrency, and given that a non-concurrent
index build is considerably cheaper, that's a nice optimization. As well
as fixing the bug at hand?

I think it'd be really user hostile if ReindexMultipleTables() suddenly
started to error out if there were any temp tables. That clearly can't
be an option.

An extra solution I can think of is to not skip temporary tables with
on-commit actions, but just fallback to the non-concurrent path in
ReindexMultipleTables when reindexing each relation for any temporary
tables processed (all of them, and not just these with on-commit
actions actually).

Did you actually read the sub-thread before replying? That's literally
what we are discussing:

On 2019-11-13 11:45:34 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

I wonder if we instead ought to just ignore the CONCURRENTLY when
targetting a temp table? That'd be a correct optimization for temp
tables, and would fix the issue at hand...

Oh, I like that idea. Keeps applications from having to think
about this.

That's the email you directly replied to.

Greetings,

Andres Freund

On Thu, Nov 14, 2019 at 06:54:12PM -0800, Andres Freund wrote:

I think it'd be really user hostile if ReindexMultipleTables() suddenly
started to error out if there were any temp tables. That clearly can't
be an option.

Okay.

Did you actually read the sub-thread before replying? That's literally
what we are discussing:

Oh, sorry. I got confused with the thread as I was not quite clear if
you were referring to work on temp tables only for the ones with
on-commit actions or all of them. It makes sense to do so for all, as
you said.
--
Michael

On Fri, Nov 15, 2019 at 12:21:41PM +0900, Michael Paquier wrote:

On Thu, Nov 14, 2019 at 06:54:12PM -0800, Andres Freund wrote:

I think it'd be really user hostile if ReindexMultipleTables() suddenly
started to error out if there were any temp tables. That clearly can't
be an option.

Okay.

So, here is a patch to address all that. I have also added a test for
REINDEX SCHEMA on a temporary schema. While looking at the problem, I
have found a crash if trying to reindex concurrently an index or a
table using a temporary relation from a different session because we
have been lacking checks with RELATION_IS_OTHER_TEMP() in the
concurrent code paths. For a schema or database reindex this was not
happening as these are filtered out using isTempNamespace(). Please
note that I have not changed index_drop() for the concurrent mode.
Per my checks this does not actually cause any direct bugs as this
code path just takes care of dropping the dependencies with the index.
There could be an argument for changing that on HEAD though, but I am
not sure that this is worth the complication either.
--
Michael

Hi Tom, Andres,

On Fri, Nov 15, 2019 at 05:07:06PM +0900, Michael Paquier wrote:

So, here is a patch to address all that. I have also added a test for
REINDEX SCHEMA on a temporary schema. While looking at the problem, I
have found a crash if trying to reindex concurrently an index or a
table using a temporary relation from a different session because we
have been lacking checks with RELATION_IS_OTHER_TEMP() in the
concurrent code paths. For a schema or database reindex this was not
happening as these are filtered out using isTempNamespace(). Please
note that I have not changed index_drop() for the concurrent mode.
Per my checks this does not actually cause any direct bugs as this
code path just takes care of dropping the dependencies with the index.
There could be an argument for changing that on HEAD though, but I am
not sure that this is worth the complication either.

Would you prefer to look at the patch once? I am planning to review
it once again in one or two days and commit it, unless there are
objections. One thing I am planning to add in the tests are small
checks to make sure that the index relfilenodes have been properly
updated for the temporary tables.
--
Michael

Hi,

I'm on vacation till early December, so I'll not respond quickly....

On 2019-11-15 17:07:06 +0900, Michael Paquier wrote:

So, here is a patch to address all that. I have also added a test for
REINDEX SCHEMA on a temporary schema. While looking at the problem, I
have found a crash if trying to reindex concurrently an index or a
table using a temporary relation from a different session because we
have been lacking checks with RELATION_IS_OTHER_TEMP() in the
concurrent code paths.

Probably worth fixing separately?

Please note that I have not changed index_drop() for the concurrent
mode. Per my checks this does not actually cause any direct bugs as
this code path just takes care of dropping the dependencies with the
index. There could be an argument for changing that on HEAD though,
but I am not sure that this is worth the complication either.

I'm extremely doubtful this works correctly. E.g., what prevents the
tids for index_concurrently_set_dead() from being recycled and pointing
to a different row after one of the internal transactions?

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 374e2d0efe..7de36ca7e2 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -550,6 +550,15 @@ DefineIndex(Oid relationId,
lockmode = stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock;
rel = table_open(relationId, lockmode);

+	/*
+	 * Ignore concurrent index creation for temporary tables.  Such
+	 * relations only work with the current session, so they are not
+	 * subject to concurrency problems.  Using a non-concurrent build
+	 * is also more performant.
+	 */
+	if (rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
+		stmt->concurrent = false;

I don't think "ignore concurrent index creation" is a good description -
they're still created. I'd rephrase it as "For temporary tables build
index non-concurrently", or something in that vein.

I think we also need to mention somewhere that it's actually crucial to
ignore them, as otherwise we'd run into problems with on commit truncate
/ drop.

Probably worthwhile to centralize check and comments into one place, to
avoid duplication / them diverging. Perhaps something like
IndexCreationSupportsConcurrent() or IndexCreationForceNonConcurrent()?

@@ -2771,6 +2797,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
/* Open relation to get its indexes */
heapRelation = table_open(relationOid, ShareUpdateExclusiveLock);

+				/* Temporary tables are not processed concurrently */
+				Assert(heapRelation->rd_rel->relpersistence != RELPERSISTENCE_TEMP);

s/are not/can not/?

+-- Temporary tables with concurrent builds
+CREATE TEMP TABLE concur_temp (f1 int, f2 text);
+INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
+DROP TABLE concur_temp;
+-- On-commit actions
+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 TABLE concur_temp;
--

I'd also add a test for ON COMMIT DROP.

-- Try some concurrent index drops
--

DROP INDEX CONCURRENTLY likely has nearly exactly the same problem,
right?

diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 629a31ef79..e26f450846 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -129,6 +129,9 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
&mdash; see <xref linkend="sql-createindex-concurrently"
endterm="sql-createindex-concurrently-title"/>.
</para>
+       <para>
+        This option is ignored for temporary relations.
+       </para>
</listitem>
</varlistentry>

diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 10881ab03a..e5d2b1a06e 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -162,6 +162,9 @@ REINDEX [ ( VERBOSE ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [ CONCURR
&mdash; see <xref linkend="sql-reindex-concurrently"
endterm="sql-reindex-concurrently-title"/>.
</para>
+     <para>
+      This option is ignored for temporary relations.
+     </para>
</listitem>
</varlistentry>

I think either this needs to be more verbose, explaining that there's no
harm from the option being ignored, or it should be ignored as an
implementation detail.

Greetings,

Andres Freund

On Tue, Nov 19, 2019 at 05:36:49PM -0800, Andres Freund wrote:

On 2019-11-15 17:07:06 +0900, Michael Paquier wrote:

So, here is a patch to address all that. I have also added a test for
REINDEX SCHEMA on a temporary schema. While looking at the problem, I
have found a crash if trying to reindex concurrently an index or a
table using a temporary relation from a different session because we
have been lacking checks with RELATION_IS_OTHER_TEMP() in the
concurrent code paths.

Thanks for providing comments. The next set of minor releases is in
February, so we have some room until that. For now I'll just add this
patch to the next CF as a bug fix to keeo track of it.

Probably worth fixing separately?

There is already a check for RELATION_IS_OTHER_TEMP() in the
non-concurrent reindex path, so by forcing temp relations to take this
path then the issue gets fixed automatically, with the error message
from reindex_index() generated.

Please note that I have not changed index_drop() for the concurrent
mode. Per my checks this does not actually cause any direct bugs as
this code path just takes care of dropping the dependencies with the
index. There could be an argument for changing that on HEAD though,
but I am not sure that this is worth the complication either.

I'm extremely doubtful this works correctly. E.g., what prevents the
tids for index_concurrently_set_dead() from being recycled and pointing
to a different row after one of the internal transactions?

ON COMMIT DELETE ROWS does a physical truncation of the relation
files. And as DROP INDEX CONCURRENTLY cannot be run in a transaction
block, you would never face a case where you have no past TIDs which
could be referred to when setting the index as invalid. Now I don't
actually object to enforce the non-concurrent path in index_drop() for
*all* temporary relations. Anyway, that makes sense in itself on
performance grounds, similarly to the create path, so did that by
enforcing the flag in index_drop() (doDeletion would be tempting but I
took the problem at its root). And added some tests for the drop path
and an extra assertion.

I don't think "ignore concurrent index creation" is a good description -
they're still created. I'd rephrase it as "For temporary tables build
index non-concurrently", or something in that vein.

I think we also need to mention somewhere that it's actually crucial to
ignore them, as otherwise we'd run into problems with on commit truncate
/ drop.

Sure. I have expanded the comment section on that.

Probably worthwhile to centralize check and comments into one place, to
avoid duplication / them diverging. Perhaps something like
IndexCreationSupportsConcurrent() or IndexCreationForceNonConcurrent()?

Good idea, done that. It reduces the explanations of the patch to be
in a single location.

@@ -2771,6 +2797,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
/* Open relation to get its indexes */
heapRelation = table_open(relationOid, ShareUpdateExclusiveLock);

+				/* Temporary tables are not processed concurrently */
+				Assert(heapRelation->rd_rel->relpersistence != RELPERSISTENCE_TEMP);

s/are not/can not/?

Okay.

+-- Temporary tables with concurrent builds
+CREATE TEMP TABLE concur_temp (f1 int, f2 text);
+INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
+DROP TABLE concur_temp;
+-- On-commit actions
+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 TABLE concur_temp;
--

I'd also add a test for ON COMMIT DROP.

Considered that, but ON COMMIT DROP does not make sense because it
requires a transaction context, which is why I did not add one. And
it seems to me that there is not much value to just check after CIC or
REINDEX's restriction to not run in a transaction block? I added
tests for these two, but I am of the opinion that they don't bring
much.

diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 10881ab03a..e5d2b1a06e 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -162,6 +162,9 @@ REINDEX [ ( VERBOSE ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [ CONCURR
&mdash; see <xref linkend="sql-reindex-concurrently"
endterm="sql-reindex-concurrently-title"/>.
</para>
+     <para>
+      This option is ignored for temporary relations.
+     </para>
</listitem>
</varlistentry>

I think either this needs to be more verbose, explaining that there's no
harm from the option being ignored, or it should be ignored as an
implementation detail.

I think that documenting it is good for the end-user as well. Just
attempted something in the updated version for all the sections of the
docs involved.
--
Michael

On Wed, Nov 20, 2019 at 12:54:08PM +0900, Michael Paquier wrote:

Thanks for providing comments. The next set of minor releases is in
February, so we have some room until that. For now I'll just add this
patch to the next CF as a bug fix to keeo track of it.

Andres, do you have plans to look at this patch? Fixing it by the
next minor release would be nice, so we still have time.
--
Michael

Hi,

On 2019-12-09 17:06:30 +0900, Michael Paquier wrote:

On Wed, Nov 20, 2019 at 12:54:08PM +0900, Michael Paquier wrote:

Thanks for providing comments. The next set of minor releases is in
February, so we have some room until that. For now I'll just add this
patch to the next CF as a bug fix to keeo track of it.

Andres, do you have plans to look at this patch? Fixing it by the
next minor release would be nice, so we still have time.

Looking now.

Greetings,

Andres Freund

On 2019-Nov-20, Michael Paquier wrote:

diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 1113d25b2d..04d3d4826f 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -113,6 +113,8 @@ extern bool CompareIndexInfo(IndexInfo *info1, IndexInfo *info2,

extern void BuildSpeculativeIndexInfo(Relation index, IndexInfo *ii);

+extern bool RelationSupportsConcurrently(char relpersistence);
+
extern void FormIndexDatum(IndexInfo *indexInfo,
TupleTableSlot *slot,
EState *estate,

I liked Andres' original naming suggestion better FWIW. With this, one
wonders "concurrently what?"

+/*
+ * RelationSupportsConcurrently
+ *
+ * Check if a relation supports concurrent builds or not.  This is
+ * used as a sanity check prior processing CREATE INDEX, DROP INDEX
+ * or REINDEX when using CONCURRENTLY.
+ */

Some suggestions,
"RelationSupportsConcurrentIndexing" or
"IndexSupportsConcurrently". Maybe replace the "ing" in the first or
"ly" in the second with "DDL" or "Ops". (Also, if it's just about
indexes and appears in index.h, why did you use the prefix "Relation"?)

In the indexcmds.c Reindex* routines, why not turn off the "concurrent"
flag?

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 2019-11-20 12:54:08 +0900, Michael Paquier wrote:

On Tue, Nov 19, 2019 at 05:36:49PM -0800, Andres Freund wrote:

Please note that I have not changed index_drop() for the concurrent
mode. Per my checks this does not actually cause any direct bugs as
this code path just takes care of dropping the dependencies with the
index. There could be an argument for changing that on HEAD though,
but I am not sure that this is worth the complication either.

I'm extremely doubtful this works correctly. E.g., what prevents the
tids for index_concurrently_set_dead() from being recycled and pointing
to a different row after one of the internal transactions?

ON COMMIT DELETE ROWS does a physical truncation of the relation
files. And as DROP INDEX CONCURRENTLY cannot be run in a transaction
block, you would never face a case where you have no past TIDs which
could be referred to when setting the index as invalid.

It's probably not reachable, but it strikes me as really fragile and
dangerous. If e.g. somehow ON COMMIT DROP tables could exist when DROP
CONCURRENTLY were run, the index_concurrently_set_dead() could very well
target a row that's since been deleted in an earlier transaction.

Now I don't actually object to enforce the non-concurrent path in
index_drop() for *all* temporary relations. Anyway, that makes sense
in itself on performance grounds, similarly to the create path, so did
that by enforcing the flag in index_drop() (doDeletion would be
tempting but I took the problem at its root). And added some tests
for the drop path and an extra assertion.

Cool.

I still think we'd be well served to add a few CheckTableNotInUse() type
checks...

+-- Temporary tables with concurrent builds
+CREATE TEMP TABLE concur_temp (f1 int, f2 text);
+INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
+DROP TABLE concur_temp;
+-- On-commit actions
+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 TABLE concur_temp;
--

I'd also add a test for ON COMMIT DROP.

Considered that, but ON COMMIT DROP does not make sense because it
requires a transaction context, which is why I did not add one. And
it seems to me that there is not much value to just check after CIC or
REINDEX's restriction to not run in a transaction block? I added
tests for these two, but I am of the opinion that they don't bring
much.

I think because CIC now falls back to non-concurrent mode, it's
worthwhile to exercise this path. It seems far from unlikely that the
code gets moved around enough that suddenly CIC is allowed in
transactions when targetting temp tables.

diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 10881ab03a..e5d2b1a06e 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -162,6 +162,9 @@ REINDEX [ ( VERBOSE ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [ CONCURR
&mdash; see <xref linkend="sql-reindex-concurrently"
endterm="sql-reindex-concurrently-title"/>.
</para>
+     <para>
+      This option is ignored for temporary relations.
+     </para>
</listitem>
</varlistentry>

I think either this needs to be more verbose, explaining that there's no
harm from the option being ignored, or it should be ignored as an
implementation detail.

I think that documenting it is good for the end-user as well.

Why?

Just attempted something in the updated version for all the sections
of the docs involved. -- Michael

diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 1113d25b2d..04d3d4826f 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -113,6 +113,8 @@ extern bool CompareIndexInfo(IndexInfo *info1, IndexInfo *info2,

extern void BuildSpeculativeIndexInfo(Relation index, IndexInfo *ii);

+extern bool RelationSupportsConcurrently(char relpersistence);
+
extern void FormIndexDatum(IndexInfo *indexInfo,
TupleTableSlot *slot,
EState *estate,
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 67f637de11..0012ebf69c 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2017,6 +2017,13 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode)
LOCKTAG		heaplocktag;
LOCKMODE	lockmode;

+	/*
+	 * Enforce non-concurrent drop if the relation does not support this
+	 * option.
+	 */
+	if (!RelationSupportsConcurrently(get_rel_persistence(indexId)))
+		concurrent = false;
+

Echoing Alvaro, I'm less than convinced by this name.

+	/*
+	 * Enforce non-concurrent build if the relation does not support this
+	 * option.
+	 */
+	if (!RelationSupportsConcurrently(rel->rd_rel->relpersistence))
+		stmt->concurrent = false;
+

Copying this to some, but not all, the places where
RelationSupportsConcurrently() is called doesn't seem helpful...

--- 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 EXISTS ] <replaceable class=
&mdash; see <xref linkend="sql-createindex-concurrently"
endterm="sql-createindex-concurrently-title"/>.
</para>
+       <para>
+        This option is ignored for temporary relations as such relations
+        are assigned to the session using them, so they are not subject to
+        problems with concurrent locking issues.
+       </para>
</listitem>
</varlistentry>

This strikes me as confusing to users. They won't understand "concurrent
locking issues" (nor am I sure that I really know what precisely that
means). And "temporary relations as such relations are assigned" is also
confusing.

If we want to add docs, I'd say at most something like "For temporary
tables index creation is always non-concurrent, as no other session can
access them, and non-concurrent index creation is cheaper.".

Greetings,

Andres Freund

On Thu, Dec 12, 2019 at 05:11:08PM -0300, Alvaro Herrera wrote:

I liked Andres' original naming suggestion better FWIW. With this, one
wonders "concurrently what?"

I did not like the "creation" part from the original suggestion :)
IndexCreationSupportsConcurrent() called from a place where an index
is dropped does not sound very consistent.

Some suggestions,
"RelationSupportsConcurrentIndexing" or
"IndexSupportsConcurrently". Maybe replace the "ing" in the first or
"ly" in the second with "DDL" or "Ops". (Also, if it's just about
indexes and appears in index.h, why did you use the prefix "Relation"?)

RelationSupportsConcurrentIndexing sounds like a good compromise to
me. The reasoning behind using relation is that this check can be
used for an index or its parent relation.
--
Michael