Re: REINDEX CONCURRENTLY unexpectedly fails

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