Re: REINDEX SCHEMA/DATABASE/SYSTEM weak with dropped relations

On 13.08.2020 07:38, Michael Paquier wrote:
> Hi all,
>
> While working on support for REINDEX for partitioned relations, I have
> noticed an old bug in the logic of ReindexMultipleTables(): the list
> of relations to process is built in a first transaction, and then each
> table is done in an independent transaction, but we don't actually
> check that the relation still exists when doing the real work.  I
> think that a complete fix involves two things:
> 1) Addition of one SearchSysCacheExists1() at the beginning of the
> loop processing each item in the list in ReindexMultipleTables().
> This would protect from a relation dropped, but that would not be
> enough if ReindexMultipleTables() is looking at a relation dropped
> before we lock it in reindex_table() or ReindexRelationConcurrently().
> Still that's simple, cheaper, and would protect from most problems.
> 2) Be completely water-proof and adopt a logic close to what we do for
> VACUUM where we try to open a relation, but leave if it does not
> exist.  This can be achieved with just some try_relation_open() calls
> with the correct lock used, and we also need to have a new
> REINDEXOPT_* flag to control this behavior conditionally.
>
> 2) and 1) are complementary, but 2) is invasive, so based on the lack
> of complaints we have seen that does not seem really worth
> backpatching to me, and I think that we could also just have 1) on
> stable branches as a minimal fix, to take care of most of the
> problems that could show up to users.
>
> Attached is a patch to fix all that, with a cheap isolation test that
> fails on HEAD with a cache lookup failure.  I am adding that to the
> next CF for now, and I would rather fix this issue before moving on
> with REINDEX for partitioned relations as fixing this issue reduces
> the use of session locks for partition trees.
>
> Any thoughts?
> --
> Michael

Hi,
I reviewed the patch. It does work and the code is clean and sane. It 
implements a logic that we already had in CLUSTER, so I think it was 
simply an oversight. Thank you for fixing this.

I noticed that REINDEXOPT_MISSING_OK can be passed to the TOAST table 
reindex. I think it would be better to reset the flag in this 
reindex_relation() call, as we don't expect a concurrent DROP here.

     /*
      * If the relation has a secondary toast rel, reindex that too while we
      * still hold the lock on the main table.
      */
     if ((flags & REINDEX_REL_PROCESS_TOAST) && OidIsValid(toast_relid))
         result |= reindex_relation(toast_relid, flags, options);


-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company