Subject: Re: CREATE INDEX CONCURRENTLY on partitioned index
To: Justin Pryzby <pryzby@telsasoft.com>,
 Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>, =?UTF-8?B?5p2O5p2wKOaFjui/vSk=?=
 <adger.lj@alibaba-inc.com>,
 pgsql-hackers <pgsql-hackers@lists.postgresql.org>,
 =?UTF-8?B?5pu+5paH5peMKOS5ieS7jik=?= <wenjing.zwj@alibaba-inc.com>,
 Alvaro Herrera <alvherre@2ndquadrant.com>
References: <20201031063117.GF3080@telsasoft.com>
 <20201129202221.GA9477@telsasoft.com>
 <CAD21AoCHYE5=PU7kJGGUHZuYVrHTHqmpUxN+8-rLGYpdgPwJ9g@mail.gmail.com>
 <20210128143013.GC7450@telsasoft.com>
From: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
Message-ID: <be8ff634-72c6-3fc7-9175-5a6b974af4a9@postgrespro.ru>
Date: Mon, 15 Feb 2021 22:07:05 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20210128143013.GC7450@telsasoft.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

On 28.01.2021 17:30, Justin Pryzby wrote:
> On Thu, Jan 28, 2021 at 09:51:51PM +0900, Masahiko Sawada wrote:
>> On Mon, Nov 30, 2020 at 5:22 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>>> On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote:
>>>> Forking this thread, since the existing CFs have been closed.
>>>> https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd
>>>>
>>>> The strategy is to create catalog entries for all tables with indisvalid=false,
>>>> and then process them like REINDEX CONCURRENTLY.  If it's interrupted, it
>>>> leaves INVALID indexes, which can be cleaned up with DROP or REINDEX, same as
>>>> CIC on a plain table.
>>>>
>>>> On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote:
>>>>> On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier wrote:
>>>>> Note that the mentioned problem wasn't serious: there was missing index on
>>>>> child table, therefor the parent index was invalid, as intended.  However I
>>>>> agree that it's not nice that the command can fail so easily and leave behind
>>>>> some indexes created successfully and some failed some not created at all.
>>>>>
>>>>> But I took your advice initially creating invalid inds.
>>>> ...
>>>>> That gave me the idea to layer CIC on top of Reindex, since I think it does
>>>>> exactly what's needed.
>>>> On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote:
>>>>> On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier wrote:
>>>>>> It would be good also to check if
>>>>>> we have a partition index tree that maps partially with a partition
>>>>>> table tree (aka no all table partitions have a partition index), where
>>>>>> these don't get clustered because there is no index to work on.
>>>>> This should not happen, since a incomplete partitioned index is "invalid".

>>>>> I had been waiting to rebase since there hasn't been any review comments and I
>>>>> expected additional, future conflicts.
>>>>>

I attempted to review this feature, but the last patch conflicts with 
the recent refactoring, so I wasn't able to test it properly.
Could you please send a new version?

Meanwhile, here are my questions about the patch:

1) I don't see a reason to change the logic here. We don't skip counting 
existing indexes when create parent index. Why should we skip them in 
CONCURRENTLY mode?

             // If concurrent, maybe this should be done after excluding 
indexes which already exist ?
pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
                                          nparts);

2) Here we access relation field after closing the relation. Is it safe?

     /* save lockrelid and locktag for below */
     heaprelid = rel->rd_lockInfo.lockRelId;

3) leaf_partitions() function only handles indexes, so I suggest to name 
it more specifically and add a comment about meaning of 'options' parameter.

4) I don't quite understand the idea of the regression test. Why do we 
expect to see invalid indexes there?
+    "idxpart_a_idx1" UNIQUE, btree (a) INVALID

5) Speaking of documentation, I think we need to add a paragraph about 
CIC on partitioned indexes which will explain that invalid indexes may 
appear and what user should do to fix them.

6) ReindexIndexesConcurrently() needs some code cleanup.

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