public inbox for [email protected]  

help / color / mirror / Atom feed

From: Justin Pryzby <[email protected]>
To: Ilya Gladyshev <[email protected]>
Cc: Alexander Pyhalov <[email protected]>
Cc: [email protected]
Cc: Masahiko Sawada <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: 李杰(慎追) <[email protected]>
Cc: 曾文旌(义从) <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Subject: Re: CREATE INDEX CONCURRENTLY on partitioned index
Date: Sun, 4 Dec 2022 13:09:35 -0600
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<CAD21AoCHYE5=PU7kJGGUHZuYVrHTHqmpUxN+8-rLGYpdgPwJ9g@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

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

view thread (28+ messages)  latest in thread

reply

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: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: CREATE INDEX CONCURRENTLY on partitioned index
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox