Re: REINDEX CONCURRENTLY unexpectedly fails

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