Обсуждение: REINDEX CONCURRENTLY unexpectedly fails

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.

Best,
Manuel

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=
>          — 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
>        — 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
>>        — 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
> >>        — 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=
>          — 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

On Thu, Dec 12, 2019 at 01:37:09PM -0800, Andres Freund wrote:
> On 2019-11-20 12:54:08 +0900, Michael Paquier wrote:
>> 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.

Hmm.  That joins with your point downthread about future changes..

>> 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...

Sure.  We have already one in drop_index, so DROP INDEX is covered, as
well as reindex_index() which is taken by all non-concurrent REINDEX
commands.  Adding one in ReindexRelationConcurrently() may make
sense..

>> 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.

That's a good point, we have no guarantee that nobody would play with
this area in the future.  Well, the patch has those tests anyway since
the last version, so I have not touched them.

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

Even if using a temporary table, the commands are not allowed within a
transaction block, but we still track them in wait events so seeing
an event related only to a non-concurrent path when using CONCURRENTLY
can be confusing.

>> +    /*
>> +     * 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.

I would really keep "Relation" in this part of the naming as this can
be used for an index or its parent table, so in the updated attached I
have gone with RelationSupportsConcurrentIndexing(), which is a
suggestion from Alvaro.

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

Not sure I follow your point here.  The following code paths are
currently checked in the patch using this routine:
- index_drop, both used by DROP INDEX and REINDEX CONCURRENTLY.  This
routine is called basically via performMultipleDeletions().  For
REINDEX CONCURRENTLY, this cannot be actually reached, but not for
DROP INDEX CONCURRENTLY.  The logic to decide which drop behavior to
choose is done in RemoveRelations().  And while we don't support
dropping multiple objects with CONCURRENTLY, we have no way to say now
for each object which lock level should be used for the drop, so it
seems safer to me now to enforce non-concurrent to be used directly in
index_drop rather than doing so at a higher level.
- ReindexIndex, ReindexTable and ReindexMultipleTables, to check if
the non-concurrent or concurrent paths need to be called for
respectively REINDEX INDEX, TABLE and SCHEMA/DATABASE/SYSTEM.
- RelationSupportsConcurrentIndexing, as the entry point for CREATE
INDEX.

+   /*
+    * Enforce non-concurrent build if the relation does not support this
+    * option.
+    */
Or are you suggesting to remove this comment from the two places where
it is used because it does not prove to help much?

> 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.".

Sounds like a better wording to me.  Documenting it still seems rather
important to me as I suspect that it could surprise some users.
--
Michael

On Fri, Dec 13, 2019 at 12:45:36PM +0900, Michael Paquier wrote:
> Sounds like a better wording to me.  Documenting it still seems rather
> important to me as I suspect that it could surprise some users.

So, are there any more comments/objections about this stuff?  I have
plans to look at that again and wrap it at the beginning of January.
--
Michael

On 23/12/2019 03:59, Michael Paquier wrote:
> +/*
> + * RelationSupportsConcurrentIndexing
> + *
> + * 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.
> + */
> +bool
> +RelationSupportsConcurrentIndexing(char relpersistence)
> +{
> +    /*
> +     * 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 (relpersistence == RELPERSISTENCE_TEMP)
> +        return false;
> +
> +    return true;
> +}
> +

The comment says "this is used as a sanity check". "Sanity check" 
implies that it should never happen, but it is perfectly normal for it 
to return false.

The caller in DefineIndex() calls RelationSupportsConcurrentIndexing() 
only after choosing the lock mode. That's fine for temporary tables, but 
if wouldn't work right if RelationSupportsConcurrentIndexing() started 
to return false for some other tables. Maybe it would be clearer to just 
check "relpersistence == RELPERSISTENCE_TEMP" directly in the callers, 
and not have the RelationSupportsConcurrentIndexing() function.

The new text in drop_index.sgml talks about index creation, copy-pasted 
from create_index.sgml.

- Heikki

On Tue, Jan 07, 2020 at 05:33:23PM +0200, Heikki Linnakangas wrote:
> The comment says "this is used as a sanity check". "Sanity check" implies
> that it should never happen, but it is perfectly normal for it to return
> false.

Fixed, thanks.

> The caller in DefineIndex() calls RelationSupportsConcurrentIndexing() only
> after choosing the lock mode. That's fine for temporary tables, but if
> wouldn't work right if RelationSupportsConcurrentIndexing() started to
> return false for some other tables. Maybe it would be clearer to just check
> "relpersistence == RELPERSISTENCE_TEMP" directly in the callers, and not
> have the RelationSupportsConcurrentIndexing() function.

The routine has the advantage of avoiding extra duplication of
comments to justify the choice of enforcing the non-concurrent path as
mentioned upthread.  So I'd rather keep it.

> The new text in drop_index.sgml talks about index creation, copy-pasted from
> create_index.sgml.

Thanks.  Fixed.

I have spent a couple of hours poking at this code, and found two
problems:
1) The error reporting for PROGRESS_CREATEIDX_COMMAND would report a
concurrent build, but that's not the case if the work happens for a
temporary table in DefineIndex(), so the call to
RelationSupportsConcurrentIndexing needs to happen before any look at
the concurrent flag is done.  That's easy enough to fix.
2) The handling of the patch within index_drop is too weak.  As
presented, the patch first locks the OID using a RangeVar.  However
for a temporary relation we would first take ShareUpdateExclusiveLock
RemoveRelations() and then upgrade to a AccessExclusiveLock in
index_drop().  I think that actually the check in index_drop() is not
necessary, and that instead we had better do three things:
a) In RangeVarCallbackForDropRelation(), if the relation is temporary,
use AccessExclusiveLock all the time, and we know the OID of the
relation here.
b) After locking the OID with the RangeVar, re-check if the relation
is temporary, and then remove PERFORM_DELETION_CONCURRENTLY is.
c) Add an assertion in index_drop() to be sure that this code path is
never invoked concurrently with a temporary relation.

I am lacking of time today, I'll continue tomorrow.
--
Michael

On Wed, Jan 08, 2020 at 05:19:30PM +0900, Michael Paquier wrote:
> I have spent a couple of hours poking at this code, and found two
> problems:
> 1) The error reporting for PROGRESS_CREATEIDX_COMMAND would report a
> concurrent build, but that's not the case if the work happens for a
> temporary table in DefineIndex(), so the call to
> RelationSupportsConcurrentIndexing needs to happen before any look at
> the concurrent flag is done.  That's easy enough to fix.
> 2) The handling of the patch within index_drop is too weak.  As
> presented, the patch first locks the OID using a RangeVar.  However
> for a temporary relation we would first take ShareUpdateExclusiveLock
> RemoveRelations() and then upgrade to a AccessExclusiveLock in
> index_drop().  I think that actually the check in index_drop() is not
> necessary, and that instead we had better do three things:
> a) In RangeVarCallbackForDropRelation(), if the relation is temporary,
> use AccessExclusiveLock all the time, and we know the OID of the
> relation here.
> b) After locking the OID with the RangeVar, re-check if the relation
> is temporary, and then remove PERFORM_DELETION_CONCURRENTLY is.
> c) Add an assertion in index_drop() to be sure that this code path is
> never invoked concurrently with a temporary relation.
>
> I am lacking of time today, I'll continue tomorrow.

Okay, so here is an updated patch fixing those issues, with several
modifications done to the patch (docs, updates for the assertions,
some redesign).  Considering again those aspects, I have come up with
the same conclusion as what's stated above, though you actually need
to make sure that it is RangeVarGetRelidExtended() that has to be
careful about the lock to use on the temporary relation, before
anything else is done.  The callback RangeVarCallbackForDropRelation()
also needs to be careful about the relation it looks at and check if
the relation supports concurrent indexing.  On the other hand, we
could also say that we don't care about lock upgrade risks when
working on temporary tables because these are not accessed by other
sessions, though that's not a sane base to rely on IMO.  A solution
involving RangeVarGetRelidExtended() feels also like a sledgehammer to
smash a peanut, because it has a wider impact.  If lock upgrade risks
are not worth bothering, this needs to be clearly documented in the
patch with more comments.

As the patch has been heavily modified, I am switching it back to
"Needs Review" for now and I'd like to discuss more about the lock
upgrade risks, particularly if it is considered worth the effort for
temporary relations.  Thoughts are welcome.
--
Michael

Hi Heikki,

On Thu, Jan 09, 2020 at 12:06:19PM +0900, Michael Paquier wrote:
> As the patch has been heavily modified, I am switching it back to
> "Needs Review" for now and I'd like to discuss more about the lock
> upgrade risks, particularly if it is considered worth the effort for
> temporary relations.  Thoughts are welcome.

You are registered as a reviewer for this patch:
https://commitfest.postgresql.org/26/2358/

Are you planning to look at it?  Do you have some thoughts to share
about what I wrote previously?

Discarding the lock upgrade considerations is possible.  Another
approach would be, instead of ignoring CONCURRENTLY for temporary
tables, to fail if the operation is run, and ignore these when a
dabatase-wide reindex concurrently happens.  This was not liked much
at the beginning of the thread though.
--
Michael

On 09/01/2020 05:06, Michael Paquier wrote:
> On Wed, Jan 08, 2020 at 05:19:30PM +0900, Michael Paquier wrote:
>> 2) The handling of the patch within index_drop is too weak.  As
>> presented, the patch first locks the OID using a RangeVar.  However
>> for a temporary relation we would first take ShareUpdateExclusiveLock
>> RemoveRelations() and then upgrade to a AccessExclusiveLock in
>> index_drop().  I think that actually the check in index_drop() is not
>> necessary, and that instead we had better do three things:
>> a) In RangeVarCallbackForDropRelation(), if the relation is temporary,
>> use AccessExclusiveLock all the time, and we know the OID of the
>> relation here.
>> b) After locking the OID with the RangeVar, re-check if the relation
>> is temporary, and then remove PERFORM_DELETION_CONCURRENTLY is.
>> c) Add an assertion in index_drop() to be sure that this code path is
>> never invoked concurrently with a temporary relation.
>>
>> I am lacking of time today, I'll continue tomorrow.
> 
> Okay, so here is an updated patch fixing those issues, with several
> modifications done to the patch (docs, updates for the assertions,
> some redesign).  Considering again those aspects, I have come up with
> the same conclusion as what's stated above, though you actually need
> to make sure that it is RangeVarGetRelidExtended() that has to be
> careful about the lock to use on the temporary relation, before
> anything else is done.  The callback RangeVarCallbackForDropRelation()
> also needs to be careful about the relation it looks at and check if
> the relation supports concurrent indexing.  On the other hand, we
> could also say that we don't care about lock upgrade risks when
> working on temporary tables because these are not accessed by other
> sessions, though that's not a sane base to rely on IMO.  A solution
> involving RangeVarGetRelidExtended() feels also like a sledgehammer to
> smash a peanut, because it has a wider impact.

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.

> If lock upgrade risks are not worth bothering, this needs to be
> clearly documented in the patch with more comments.
Yes, definitely.

- Heikki

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

On 15/01/2020 03:39, Michael Paquier wrote:
> 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.

Heh, yeah :-).

> 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?

Some comments below:

> 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 concurrent_lock_mode)
>      LOCKTAG        heaplocktag;
>      LOCKMODE    lockmode;
>  
> +    /*
> +     * 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

Or maybe decide to do non-current drop within index_drop() itself, 
instead of requiring the caller to set 'concurrent' differently for 
temporary tables?

> @@ -2490,6 +2500,30 @@ CompareIndexInfo(IndexInfo *info1, IndexInfo *info2,
>      return true;
>  }
>  
> +/*
> + * 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) == RELPERSISTENCE_TEMP)
> +        return false;
> +
> +    return true;
> +}
> +

Sorry to beat a dead hore, but I still don't like this function:

* Does it take a table OID or index OID? (Answer: both)

* There's a hidden assumption that if 
RelationSupportsConcurrentIndexing() returns false, then it's OK to 
upgrade the lock. That's true today, but if we added other conditions 
when RelationSupportsConcurrentIndexing() returned false, there would be 
trouble. It seems like a bad abstraction.

This would be better if the function was renamed to something like "Is 
it OK to upgrade a CONCURRENTLY build to non-CONCURRENTLY?", but meh. I 
understand that it's nice to have a place for this comment, so that it 
doesn't need to be repeated in so many places. But I feel that a little 
bit of repetition is better in this case. The reasoning isn't exactly 
the same for CREATE INDEX, DROP INDEX, and REINDEX anyway.

> diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.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);
>      }
>  
> +    /*
> +     * 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 = false;
> +

Is it OK to scribble on the original 'stmt' here? Doesn't seem kosher, 
although it probably works fine in practice.

> @@ -2769,6 +2778,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
>                  /* Open relation to get its indexes */
>                  heapRelation = table_open(relationOid, ShareUpdateExclusiveLock);
>  
> +                /* Relation had better support concurrent indexing */
> +                Assert(RelationSupportsConcurrentIndexing(relationOid));
> +
>                  /* Add all the valid indexes of relation to list */
>                  foreach(lc, RelationGetIndexList(heapRelation))
>                  {

Do we care whether the *table* supports concurrent indexing, rather than 
individual indexes? I guess that's academic, since you can't have 
temporary indexes on a permanent table, or vice versa.

> @@ -2937,6 +2952,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
>          heapRel = table_open(indexRel->rd_index->indrelid,
>                               ShareUpdateExclusiveLock);
>  
> +        /*
> +         * 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,

I don't understand why this is required for this patch. It seems like a 
good thing to check, I think otherwise you get an error from the 
CheckTableNotInUse() call in index_drop(), in phase 6 where the old 
indexes are dropped. But it seems unrelated to the rest of the patch. 
Maybe commit it as a separate patch?

I came up with the attached version. It seems a bit more clear to me. 
I'm not 100% wedded to this, though, so if you want to proceed based on 
your version instead, feel free. The docs and the tests are unchanged.

- Heikki

On Fri, Jan 17, 2020 at 02:53:11PM +0200, Heikki Linnakangas wrote:
> On 15/01/2020 03:39, Michael Paquier wrote:
>> 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 concurrent_lock_mode)
>>      LOCKTAG        heaplocktag;
>>      LOCKMODE    lockmode;
>> +    /*
>> +     * 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
>
> Or maybe decide to do non-current drop within index_drop() itself, instead
> of requiring the caller to set 'concurrent' differently for temporary
> tables?

A portion which stresses me with your approach (and that I actually
used in the first versions of my patch upthread), is that we have some
extra work related to PERFORM_DELETION_CONCURRENTLY being done in
dependency.c, which becomes basically useless if you enforce the flag
only in index_drop() without making sure that the root flag is set in
RemoveRelations() (see for example the top of deleteOneObject()),
and this causes the index drop to actually mix more concurrent and
non-concurrent concepts than just the lock upgrade issue.

>> +bool
>> +RelationSupportsConcurrentIndexing(Oid relid)
>> +{
>
> Sorry to beat a dead hore, but I still don't like this function:
>
> * Does it take a table OID or index OID? (Answer: both)

Yes, the persistency is inherited.  The function mentioned a relation,
so that applied to any relkind actually.  Perhaps the function should
have made that clearer with a assertion using a relkind check or
such.  But the original sounded pretty clear to me.

> * There's a hidden assumption that if RelationSupportsConcurrentIndexing()
> returns false, then it's OK to upgrade the lock. That's true today, but if
> we added other conditions when RelationSupportsConcurrentIndexing() returned
> false, there would be trouble. It seems like a bad abstraction.
>
> This would be better if the function was renamed to something like "Is it OK
> to upgrade a CONCURRENTLY build to non-CONCURRENTLY?", but meh. I understand
> that it's nice to have a place for this comment, so that it doesn't need to
> be repeated in so many places. But I feel that a little bit of repetition is
> better in this case. The reasoning isn't exactly the same for CREATE INDEX,
> DROP INDEX, and REINDEX anyway.

Okay, I see your points.  So I am fine with your line of arguments.

>> +    /*
>> +     * 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 = false;
>> +
>
> Is it OK to scribble on the original 'stmt' here? Doesn't seem kosher,
> although it probably works fine in practice.

The idea was to avoid any future issues if any code refactored in this
area passes down IndexStmt to a subroutine and uses the concurrent
flag.  I guess that would be hard to miss if using a local variable as
you do anyway..

> Do we care whether the *table* supports concurrent indexing, rather than
> individual indexes? I guess that's academic, since you can't have temporary
> indexes on a permanent table, or vice versa.

I cared about that enough, but that's a very defensive position :)
But I agree that having a check only for individual indexes would be
just but fine.

>> +        /*
>> +         * 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,
>
> I don't understand why this is required for this patch. It seems like a good
> thing to check, I think otherwise you get an error from the
> CheckTableNotInUse() call in index_drop(), in phase 6 where the old indexes
> are dropped. But it seems unrelated to the rest of the patch. Maybe commit
> it as a separate patch?

I added that per a suggestion from Andres while touching this area of
the code.  But you are right that it would make sense to remove it
from this patch, and get that committed separately.  I don't mind
starting a new thread for this matter instead of having this
discussing buried within this thread.  Does it make sense?

> @@ -943,8 +962,11 @@ 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.
> +     *
> +     * FIXME: should this check 'concurrent' or 'stmt->concurrent'? I don't
> +     * quite understand the conditions here.
>       */
> -    Assert(!OidIsValid(stmt->oldNode) || (skip_build && !stmt->concurrent));
> +    Assert(!OidIsValid(stmt->oldNode) || (skip_build && !concurrent));

[ .. thinks .. ]
It seems to me that this should be using the local variable.
SKIP_BUILD is used in some cases for ALTER TABLE, CIC and REINDEX
CONCURRENTLY (for the creation of the duplicate index entry).  oldNode
gets used in ALTER TABLE when attempting to reuse an existing index
when changing a column type for example.

> -    if (concurrent)
> +    /*
> +     * Obtain the current persistence of the existing table.  We already hold
> +     * lock on it.
> +     */
> +    heapRel = table_open(heapOid, NoLock);
> +    persistence = heapRel->rd_rel->relpersistence;
> +    table_close(heapRel, NoLock);

Why not just using get_rel_persistence() here, as done in
ReindexMultipleTables()?

> +        /* This function shouldn't be called for temporary relations. */
> +        if (indexRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
> +            elog(ERROR, "cannot reindex a temporary table concurrently");

You are right that an elog() is better than an assertion here as this
uses a catalog data.

> I came up with the attached version. It seems a bit more clear to me. I'm
> not 100% wedded to this, though, so if you want to proceed based on your
> version instead, feel free. The docs and the tests are unchanged.

Except for the part with index_drop() where I would rather do the
decision-making for the concurrent behavior in RemoveRelations()
rather than index_drop() (as I did in v4), what you have here looks
fine to me.  Would you prefer wrapping up this stuff yourself or
should I?  This needs a backpatch down to 9.4 for the CIC/DIC part.
--
Michael

On Mon, Jan 20, 2020 at 10:59:13AM +0900, Michael Paquier wrote:
> Except for the part with index_drop() where I would rather do the
> decision-making for the concurrent behavior in RemoveRelations()
> rather than index_drop() (as I did in v4), what you have here looks
> fine to me.  Would you prefer wrapping up this stuff yourself or
> should I?  This needs a backpatch down to 9.4 for the CIC/DIC part.

Same feeling after sleeping on it.  I have worked more this morning on
this stuff and I am finishing with the attached, which is a gathering
of everything that has been discussed, and based on Heikki's v5:
- Changed the part for DROP INDEX CONCURRENTLY to do the
decision-making in RemoveRelations() at the earliest stage possible.
- Removed the call to CheckTableNotInUse() in
ReindexRelationConcurrently().  Let's use a separate patch/thread for
that.
- Found one typo in the docs of REINDEX.

If there are no objections, I would like to wrap that in the next day
or so (still need to do the work for the back-branches).
--
Michael

On Tue, Jan 21, 2020 at 11:43:03AM +0900, Michael Paquier wrote:
> If there are no objections, I would like to wrap that in the next day
> or so (still need to do the work for the back-branches).

There were various conflicts across the various back-branches, but
nothing depressing either.  Committed and back-patched down to 9.4.
--
Michael