Re: 回复：how to create index concurrently on partitioned table - Mailing list pgsql-hackers

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