Re: pgsql: New files for MERGE

On 5 April 2018 at 21:02, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Thu, Apr 5, 2018 at 11:15:20AM +0100, Simon Riggs wrote:
>> On 4 April 2018 at 21:28, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> > On 4 April 2018 at 21:14, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> >
>> >>> The normal way is to make review comments that allow change. Your
>> >>> request for change of the parser data structures is fine and can be
>> >>> done, possibly by Saturday
>> >>
>> >> I did request changes, and you've so far ignored those requests.
>> >
>> > Pavan tells me he has replied to you and is working on specific changes.
>>
>> Specific changes requested have now been implemented by Pavan and
>> committed by me.
>>
>> My understanding is that he is working on a patch for Tom's requested
>> parser changes, will post on other thread.
>
> Simon, you have three committers in this thread suggesting this patch be
> reverted. Are you just going to barrel ahead with the fixes without
> addressing their emails?

PeterG confirms that the patch works and has the agreed concurrency
semantics. Separating out the code allows us to see clearly that we
have almost complete test coverage of the code and its features.

The fixes being applied are ones proposed by Andres, Tom, Andrew,
Jesper. So their emails are being addressed in full, where there is
anything to discuss and change. Yes, that work is ongoing since there
is a CF deadline. So Pavan and I are very clearly and publicly
addressing their emails and nobody is being ignored.

The architecture of the executor has been described as wrong, but at
the time that was made there was zero detail behind that claim, so
there was nothing to comment upon. I'm surprised and somewhat
suspicious that multiple people found anything with which to agree or
disagree with that suggestion; I'm also suprised that nobody else has
pointed out the lack of substance there. Given that the executor
manifestly works and has been re-engineered according to PeterG's
requests and that many performance concerns have already been
addressed prior to commit, Pavan and I were happy with it. My proposal
to commit the patch was given 5 days ahead of time and no comments
were received by anyone, not even PeterG. There was no rush and I
personally performed extensive reviews before final commit. And as
Robert has reminded me often that committers do not possess a veto
that they can simply use to remove patches, there have been no
reasonable grounds to revoke anything.

I do have sympathy with the need for good architecture and executor
design. I expressed exactly the same last year with the missing
executor elements in the partitioning patch. Notably nobody asked for
that to be revoked, not even myself knowing that the executor would
require major changes in PG11. The executor for MERGE is in radically
better shape.

I see that Andres has now posted something giving some details about
his concerns (posted last night, so Pavan and I are only now reading
it). AFAICS these are not blocking design issues, simply requests for
some moving around of the code. We will of course answer in detail on
that thread and discuss further. Given we have full test coverage it
is reasonable to imagine that can be done relatively quickly.

Given the extreme late notice of those review requests, the relative
separation of the MERGE code and the fact this is a new feature not
related to robustness, it seems reasonable to be granted an extension
to complete changes. We're currently assessing how long that might be,
but an additional week seems likely.

The project is blessed with many great developers; I do not claim to
be one myself but I am a diligent committer. It's been said that
neither Tom or Andres thought the code would be committed so neither
looked at this code, even though they both knew of my proposed commit
of the feature in January, with some caveats at that time. Obviously,
when great people see a new thing they will always see ways of doing
it better, but that doesn't mean things are below project standards.
It would be useful to have a way for all committers to signal ahead of
time what they think is likely to happen to avoid this confusion,
rather than a few private IMs between friends. I've been surprised
before by people saying "that's not going in" when they clearly
haven't even looked at the code and yet others think it is all good.

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