Re: REINDEX CONCURRENTLY unexpectedly fails

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