Re: [HACKERS] REINDEX CONCURRENTLY 2.0

Here is an updated patch, which addresses some of your issues below as
well as the earlier reported issue that comments were lost during
REINDEX CONCURRENTLY.

On 16/01/2019 09:27, Michael Paquier wrote:
> On Fri, Jan 04, 2019 at 03:18:06PM +0300, Sergei Kornilov wrote:
>> NOTICE seems unnecessary here.
>>
>> Unfortunally concurrently reindex loses comments, reproducer:
>
> Yes, the NOTICE message makes little sense.

This is existing behavior of reindex-not-concurrently.

> I am getting back in touch with this stuff. It has been some time but
> the core of the patch has not actually changed in its base concept, so
> I am still very familiar with it as the original author. There are
> even typos I may have introduced a couple of years back, like
> "contraint". I have not yet spent much time on that, but there are at
> quick glance a bunch of things that could be retouched to get pieces
> of that committable.
>
> + The concurrent index created during the processing has a name ending in
> + the suffix ccnew, or ccold if it is an old index definiton which we failed
> + to drop. Invalid indexes can be dropped using <literal>DROP INDEX</literal>,
> + including invalid toast indexes.
> This needs <literal> markups for "ccnew" and "ccold". "definiton" is
> not correct.

Fixed those.

> index_create does not actually need its extra argument with the tuple
> descriptor. I think that we had better grab the column name list from
> indexInfo and just pass that down to index_create() (patched on my
> local branch), so it is an overkill to take a full copy of the index's
> TupleDesc.

Please send a fixup patch.

> The patch, standing as-is, is close to 2k lines long, so let's cut
> that first into more pieces refactoring the concurrent build code.
> Here are some preliminary notes:
> - WaitForOlderSnapshots() could be in its own patch.
> - index_concurrently_build() and index_concurrently_set_dead() can be
> in an independent patch. set_dead() had better be a wrapper on top of
> index_set_state_flags actually which is able to set any kind of
> flags.
> - A couple of pieces in index_create() could be cut as well.

I'm not a fan of that. I had already considered all the ways in which
subparts of this patch could get committed, and some of it was
committed, so what's left now is what I thought should stay together.
The patch isn't really that big and most of it is moving code around. I
would also avoid chopping around in this patch now and focus on getting
it finished instead. The functionality seems solid, so if it's good,
let's commit it, if it's not, let's get it fixed up.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services