Re: 回复：how to create index concurrently on partitioned table

On Thu, Sep 03, 2020 at 10:02:58PM +0300, Anastasia Lubennikova wrote:
> First of all, this patch fails at cfbot:
>
> indexcmds.c:2848:7: error: variable ‘parentoid’ set but not used
> [-Werror=unused-but-set-variable]
> Oid parentoid;^

Missed this warning, thanks for pointing it out. This is an incorrect
rebase: this variable was used as a workaround to take a session lock
on the parent table to be reindexed, session lock logic existing to
prevent cache lookup errors when dropping some portions of the tree
concurrently (1d65416 as you already know). But we don't need that
anymore.

> If this guessed fix is correct, I see the problem in the patch logic. In
> reindex_partitions() we collect parent relations to pass them to
> reindex_multiple_internal(). It implicitly changes the logic from REINDEX
> INDEX to REINDEX RELATION, which is not the same, if table has more than one
> index.

Incorrect guess here. parentoid refers to the parent table of an
index, so by saving its OID in the list of things to reindex, you
would finish by reindexing all the indexes of a partition. We need to
use partoid, as returned by find_all_inheritors() for all the members
of the partition tree (relid can be a partitioned index or partitioned
table in reindex_partitions).

> 1) in create_index.sql
>
> Are these two lines intentional checks that must fail? If so, I propose to
> add a comment.
>
> REINDEX TABLE concur_reindex_part_index;
> REINDEX TABLE CONCURRENTLY concur_reindex_part_index;
>
> A few lines around also look like they were copy-pasted and need a second
> look.

You can note some slight differences though. These are test cases for
REINDEX INDEX with tables, and REINDEX TABLE with indexes. What you
are quoting here is the part for indexes with REINDEX TABLE. And
there are already comments, see:
"-- REINDEX INDEX fails for partitioned tables"
"-- REINDEX TABLE fails for partitioned indexes"

Perhaps that's not enough, so I have added some more comments to
outline that these commands fail (8 commands in total).

> 2)  This part of ReindexRelationConcurrently() is misleading.
>
> Maybe assertion is enough. It seems, that we should never reach this code
> because both ReindexTable and ReindexIndex handle those relkinds
> separately.  Which leads me to the next question.

Yes, we could use an assert, but I did not feel any strong need to
change that either for this patch.

> 3) Is there a reason, why reindex_partitions() is not inside
> ReindexRelationConcurrently() ? I think that logically it belongs there.

Yes, it simplifies the build of the index list indexIds, as there is
no need to loop back into a different routine if working on a table or
a matview.

> 4) I haven't tested multi-level partitions yet. In any case, it would be
> good to document this behavior explicitly.

Not sure what addition we could do here. The patch states that each
partition of the partitioned relation defined gets reindexed, which
implies that this handles multiple layers automatically.

> I need a bit more time to review this patch more thoroughly. Please, wait
> for it, before committing.

Glad to hear that, please take the time you need.
--
Michael