Date: Sun, 4 Dec 2022 13:09:35 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
Cc: Alexander Pyhalov <a.pyhalov@postgrespro.ru>,
	pgsql-hackers@lists.postgresql.org,
	Masahiko Sawada <sawada.mshk@gmail.com>,
	Michael Paquier <michael@paquier.xyz>,
	=?utf-8?B?5p2O5p2wKOaFjui/vSk=?= <adger.lj@alibaba-inc.com>,
	=?utf-8?B?5pu+5paH5peMKOS5ieS7jik=?= <wenjing.zwj@alibaba-inc.com>,
	Alvaro Herrera <alvherre@2ndquadrant.com>
Subject: Re: CREATE INDEX CONCURRENTLY on partitioned index
Message-ID: <20221204190935.GD14156@telsasoft.com>
References: <20201129202221.GA9477@telsasoft.com>
 <CAD21AoCHYE5=PU7kJGGUHZuYVrHTHqmpUxN+8-rLGYpdgPwJ9g@mail.gmail.com>
 <20210128143013.GC7450@telsasoft.com>
 <be8ff634-72c6-3fc7-9175-5a6b974af4a9@postgrespro.ru>
 <20210226182019.GU20769@telsasoft.com>
 <04657227cc37b6353cf0c72bedac70cc@postgrespro.ru>
 <20220628183309.GD28130@telsasoft.com>
 <bec008ae632d54b3d81af2ca86419732@postgrespro.ru>
 <20221121030011.GU11463@telsasoft.com>
 <5bafee07d0e1b3ce5359d1656c90b4836d7cb6a1.camel@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="zCKi3GIZzVBPywwA"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5bafee07d0e1b3ce5359d1656c90b4836d7cb6a1.camel@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20221204190935.GD14156%40telsasoft.com>
Precedence: bulk


--zCKi3GIZzVBPywwA
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Sat, Dec 03, 2022 at 07:13:30PM +0400, Ilya Gladyshev wrote:
> Hi,
> 
> Thank you Justin and Alexander for working on this, I have reviewed and
> tested the latest patch, it works well, the problems mentioned
> previously are all fixed. I like the idea of sharing code of reindex
> and index, but I have noticed some peculiarities as a user. 
> 
> The reporting is somewhat confusing as it switches to reporting for
> reindex concurrently while building child indexes, this should be fixed
> with the simple patch I have attached. Another thing that I have
> noticed is that REINDEX, which is used under the hood, creates new
> indexes with suffix _ccnew, and if the index building fails, the
> indexes that could not be build will have the name with _ccnew suffix.
> This can actually be seen in your test:
> 
> ERROR:  could not create unique index "idxpart2_a_idx2_ccnew"

> I find it quite confusing and I don't think that this the expected
> behavior (if it is, I think it should be documented, like it is for
> REINDEX). As an example of problems that it might entail, DROP INDEX
> will not drop all the invalid indexes in the inheritance tree, because
> it will leave _ccnew indexes in place, which is ok for reindex
> concurrently, but that's not how C-I-C works now. I think that fixing
> this problem requires some heavy code rewrite and I'm not quite sure

This beavior is fixed.  I re-factored and re-implented to use
DefineIndex() for building indexes concurrently rather than reindexing.
That makes the patch smaller, actually, and has the added benefit of
splitting off the "Concurrently" part of DefineIndex() into a separate
function.

This currently handles partitions with a loop around the whole CIC
implementation, which means that things like WaitForLockers() happen
once for each index, the same as REINDEX CONCURRENTLY on a partitioned
table.  Contrast that with ReindexRelationConcurrently(), which handles
all the indexes on a table in one pass by looping around indexes within
each phase.

BTW, it causes the patch to fail to apply in cfbot when you send an
additional (002) supplementary patch without including the original
(001) patch.  You can name it *.txt to avoid the issue.
https://wiki.postgresql.org/wiki/Cfbot#Which_attachments_are_considered_to_be_patches.3F

Thanks for looking.

-- 
Justin

--zCKi3GIZzVBPywwA
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-table.patch"