On 5 April 2018 at 21:02, Bruce Momjian <br...@momjian.us> wrote: > On Thu, Apr 5, 2018 at 11:15:20AM +0100, Simon Riggs wrote: >> On 4 April 2018 at 21:28, Simon Riggs <si...@2ndquadrant.com> wrote: >> > On 4 April 2018 at 21:14, Andres Freund <and...@anarazel.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
