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

From: Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)ru>
To: Michael Paquier <michael(at)paquier(dot)xyz>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: REINDEX SCHEMA/DATABASE/SYSTEM weak with dropped relations
Date: 2020-08-31 15:10:46
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

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:
The Russian Postgres Company

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2020-08-31 15:20:57 Re: list of extended statistics on psql
Previous Message Bruce Momjian 2020-08-31 15:03:45 Re: file_fdw vs relative paths