Re: REINDEX CONCURRENTLY unexpectedly fails

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