From: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: 李杰(慎追) <adger.lj@alibaba-inc.com>
Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>
Cc: 曾文旌(义从) <wenjing.zwj@alibaba-inc.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Subject: Re: 回复：how to create index concurrently on partitioned table
Date: Thu, 3 Sep 2020 22:02:58 +0300
Message-ID: <4cefde7e-cc68-2e0c-077b-d6fe3a220344@postgrespro.ru> (raw)
In-Reply-To: <20200902013944.GF1489@paquier.xyz>
References: <20200618024114.GF118592@paquier.xyz>
<20200616010221.GA2353@paquier.xyz>
<20200615123742.GL52676@paquier.xyz>
<20200808063744.GG20473@telsasoft.com>
<20200809050009.GA17986@paquier.xyz>
<20200809234423.GH20473@telsasoft.com>
<20200812045438.GA11663@paquier.xyz>
<20200812052820.GW20473@telsasoft.com>
<20200812133708.GB11663@paquier.xyz>
<20200814002945.GB2057@paquier.xyz>
<20200902013944.GF1489@paquier.xyz>
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.
inline patch 1: src/backend/commands/indexcmds.c (+1/-1 lines) download inline-1.patchdiff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.lubennikova@postgrespro.ru, michael@paquier.xyz, pryzby@telsasoft.com, adger.lj@alibaba-inc.com, pgsql-hackers@lists.postgresql.org, wenjing.zwj@alibaba-inc.com, alvherre@2ndquadrant.com
Subject: Re: 回复：how to create index concurrently on partitioned table
In-Reply-To: <4cefde7e-cc68-2e0c-077b-d6fe3a220344@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox