Re: REINDEX CONCURRENTLY unexpectedly fails

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