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

On 02.09.2020 04:39, Michael Paquier wrote:
>
> The problem with dropped relations in REINDEX has been addressed by
> 1d65416, so I have gone through this patch again and simplified the
> use of session locks, these being taken only when doing a REINDEX
> CONCURRENTLY for a given partition. This part is in a rather
> committable shape IMO, so I would like to get it done first, before
> looking more at the other cases with CIC and CLUSTER. I am still
> planning to go through it once again.
> --
> Michael

Thank you for advancing this work.

I was reviewing the previous version, but the review became outdated
before I sent it. Overall design is fine, but I see a bunch of things
that need to be fixed before commit.

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;^

It seems to be just a typo. With this minor fix the patch compiles and
passes tests.

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 75008eebde..f5b3c53a83 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2864,7 +2864,7 @@ reindex_partitions(Oid relid, int options, bool concurrent,

/* Save partition OID */
old_context = MemoryContextSwitchTo(reindex_context);
- partitions = lappend_oid(partitions, partoid);
+ partitions = lappend_oid(partitions, parentoid);
MemoryContextSwitchTo(old_context);
}

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. For example, if I add one more index to a
partitioned table from a create_index.sql test:

create index idx on concur_reindex_part_0_2 using btree (c2);

and call

REINDEX INDEX CONCURRENTLY concur_reindex_part_index;

idx will be reindexed as well. I doubt that it is the desired behavior.

A few more random issues, that I noticed at first glance:

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.

2)  This part of ReindexRelationConcurrently() is misleading.

        case RELKIND_PARTITIONED_TABLE:
        case RELKIND_PARTITIONED_INDEX:
        default:
            /* Return error if type of relation is not supported */
            ereport(ERROR,
                    (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                     errmsg("cannot reindex this type of relation concurrently")));

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.

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

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

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

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