Re: [HACKERS] REINDEX CONCURRENTLY 2.0

On Tue, Jan 29, 2019 at 09:51:35PM +0100, Peter Eisentraut wrote:
> On 16/01/2019 09:27, Michael Paquier wrote:
>> 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.

Sure. Attached is a patch which can be applied on top of what you
sent last, based on what I noticed at review, here and there. You
also forgot to switch two heap_open() to table_open() in pg_depend.c.

>> 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.

Well, the feature looks solid to me, and not much of its code has
actually changed over the years FWIW.

Committing large and complex patches is something you have more
experience with than myself, and I find the exercise very difficult.
So if you feel it's adapted to keep things grouped together, it is not
an issue to me. I'll follow the lead.
--
Michael