Thread: Re: pgsql: New files for MERGE

Hi,

On 2018-04-03 09:24:12 +0000, Simon Riggs wrote:
> New files for MERGE
> src/backend/executor/nodeMerge.c                   |  575 +++++++
> src/backend/parser/parse_merge.c                   |  660 ++++++++
> src/include/executor/nodeMerge.h                   |   22 +
> src/include/parser/parse_merge.h                   |   19 +

Getting a bit grumpy here.  So you pushed this, without responding in
any way to the objections I made in
http://archives.postgresql.org/message-id/20180403021800.b5nsgiclzanobiup%40alap3.anarazel.de
and did it in a manner that doesn't even compile?

Greetings,

Andres Freund

Hi,

On 2018-04-03 08:32:45 -0700, Andres Freund wrote:
> Hi,
> 
> On 2018-04-03 09:24:12 +0000, Simon Riggs wrote:
> > New files for MERGE
> > src/backend/executor/nodeMerge.c                   |  575 +++++++
> > src/backend/parser/parse_merge.c                   |  660 ++++++++
> > src/include/executor/nodeMerge.h                   |   22 +
> > src/include/parser/parse_merge.h                   |   19 +
> 
> Getting a bit grumpy here.  So you pushed this, without responding in
> any way to the objections I made in
> http://archives.postgresql.org/message-id/20180403021800.b5nsgiclzanobiup%40alap3.anarazel.de
> and did it in a manner that doesn't even compile?

This needs at the very least a response to the issues pointed out in the
referenced email that you chose to ignore without any sort of comment.

Greetings,

Andres Freund

Hi,

On 2018-04-03 08:32:45 -0700, Andres Freund wrote:
> Hi,
> 
> On 2018-04-03 09:24:12 +0000, Simon Riggs wrote:
> > New files for MERGE
> > src/backend/executor/nodeMerge.c                   |  575 +++++++
> > src/backend/parser/parse_merge.c                   |  660 ++++++++
> > src/include/executor/nodeMerge.h                   |   22 +
> > src/include/parser/parse_merge.h                   |   19 +
> 
> Getting a bit grumpy here.  So you pushed this, without responding in
> any way to the objections I made in
> http://archives.postgresql.org/message-id/20180403021800.b5nsgiclzanobiup%40alap3.anarazel.de
> and did it in a manner that doesn't even compile?

This needs at the very least a response to the issues pointed out in the
referenced email that you chose to ignore without any sort of comment.

Greetings,

Andres Freund

Hi,

On 2018-04-05 00:02:06 +0530, Pavan Deolasee wrote:
> Apologies from my end. Simon checked with me regarding your referenced
> email. I was in the middle of responding to it (with a add-on patch to take
> care of your review comments), but got side tracked by some high priority
> customer escalation. I shall respond soon.

Hows that an explanation for just going ahead and committing? Without
even commenting on why one thinks the pointed out issues are something
that can be resolved later or somesuch?  This has an incredibly rushed
feel to it.

Greetings,

Andres Freund

Hi,

On 2018-04-05 00:02:06 +0530, Pavan Deolasee wrote:
> Apologies from my end. Simon checked with me regarding your referenced
> email. I was in the middle of responding to it (with a add-on patch to take
> care of your review comments), but got side tracked by some high priority
> customer escalation. I shall respond soon.

Hows that an explanation for just going ahead and committing? Without
even commenting on why one thinks the pointed out issues are something
that can be resolved later or somesuch?  This has an incredibly rushed
feel to it.

Greetings,

Andres Freund

On Wed, Apr 4, 2018 at 11:46 AM, Andres Freund <andres@anarazel.de> wrote:
> Hows that an explanation for just going ahead and committing? Without
> even commenting on why one thinks the pointed out issues are something
> that can be resolved later or somesuch?  This has an incredibly rushed
> feel to it.

I agree that this was handled in a way that was just unsatisfactory.
It was also unnecessary, since we still have a few days left until
feature freeze.

-- 
Peter Geoghegan

On Wed, Apr 4, 2018 at 11:46 AM, Andres Freund <andres@anarazel.de> wrote:
> Hows that an explanation for just going ahead and committing? Without
> even commenting on why one thinks the pointed out issues are something
> that can be resolved later or somesuch?  This has an incredibly rushed
> feel to it.

I agree that this was handled in a way that was just unsatisfactory.
It was also unnecessary, since we still have a few days left until
feature freeze.

-- 
Peter Geoghegan

[ removing -committers from cc ]

Pavan Deolasee <pavan.deolasee@gmail.com> writes:
> On Thu, Apr 5, 2018 at 12:16 AM, Andres Freund <andres@anarazel.de> wrote:
>> Hows that an explanation for just going ahead and committing? Without
>> even commenting on why one thinks the pointed out issues are something
>> that can be resolved later or somesuch?  This has an incredibly rushed
>> feel to it.

> Anyways, I think your reviews comments are useful and I've incorporated
> most of those. Obviously certain things like creating a complete new
> executor machinery is not practical given where we're in the release cycle
> and I am not sure if that has any significant advantages over what we have
> today.

Well, what's on the table is reverting this patch and asking you to try
again in the v12 cycle.  Given Andres' concerns about the executor design,
and mine about the way the parsing end is built, there's certainly no way
that that's all getting fixed by Saturday.  Given pretty much everybody's
unhappiness with the way this patch was forced through at the last minute,
I do not think you should expect that we'll say, "okay, we'll let you ship
a bad version of MERGE because there's no more time in this cycle".

Personally, I didn't think we had consensus on whether the semantics
are right, let alone on whether this is a satisfactory implementation
code-wise.  I know I've never looked at the patch before today; I did not
think it was close enough to being committed that I would need to.

            regards, tom lane

On Wed, Apr 4, 2018 at 12:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Personally, I didn't think we had consensus on whether the semantics
> are right, let alone on whether this is a satisfactory implementation
> code-wise.  I know I've never looked at the patch before today; I did not
> think it was close enough to being committed that I would need to.

To be fair, I was happy with the semantics we came up with for READ
COMMITTED conflict handling, although it wasn't that long ago that
that ceased to be the big concern. This happened due to a truly heroic
effort from Pavan.

The problems that remained were with the representation used during
parsing, planning, and execution, which seem like they could have a
lot of unforeseen consequences. Plus a general lack of maturity.
Things like column-level privileges were broken as recently as a week
before commit, due to being totally untested. That was a consequence
of the representation in the executor.

-- 
Peter Geoghegan

On 4 April 2018 at 20:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> [ removing -committers from cc ]
>
> Pavan Deolasee <pavan.deolasee@gmail.com> writes:
>> On Thu, Apr 5, 2018 at 12:16 AM, Andres Freund <andres@anarazel.de> wrote:
>>> Hows that an explanation for just going ahead and committing? Without
>>> even commenting on why one thinks the pointed out issues are something
>>> that can be resolved later or somesuch?  This has an incredibly rushed
>>> feel to it.
>
>> Anyways, I think your reviews comments are useful and I've incorporated
>> most of those. Obviously certain things like creating a complete new
>> executor machinery is not practical given where we're in the release cycle
>> and I am not sure if that has any significant advantages over what we have
>> today.
>
> Well, what's on the table is reverting this patch and asking you to try
> again in the v12 cycle.  Given Andres' concerns about the executor design,
> and mine about the way the parsing end is built, there's certainly no way
> that that's all getting fixed by Saturday.  Given pretty much everybody's
> unhappiness with the way this patch was forced through at the last minute,
> I do not think you should expect that we'll say, "okay, we'll let you ship
> a bad version of MERGE because there's no more time in this cycle".

This version works, with agreed semantics, all fully tested and documented.

It's also neat and tight. Look how easy it was for Peter to add WITH
semantics on top of it.

And it's isolated, so its not a threat to anybody that doesn't choose
to use it. Users want it and will use this; if I didn't know that for
certain I wouldn't spend time on it.

> Personally, I didn't think we had consensus on whether the semantics
> are right, let alone on whether this is a satisfactory implementation
> code-wise.  I know I've never looked at the patch before today; I did not
> think it was close enough to being committed that I would need to.

I have rejected patches in the past for clearly stated reasons that
affect users. I regret that those people didn't discuss their designs
before they posted patches and serious holes were found. And in
response I provided clear design outlines of what was needed. That is
not what is happening here.

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

If saying "I'm unhappy with something" is sufficient grounds for
rejecting a patch, I'm surprised to hear it. There has been no
discussion of what exactly would be better, only that what we have is
somehow wrong, a point which both Pavan and I dispute, not least
because the executor has already been rewritten once at Peter's
request.

I was under no pressure at all to commit this. In my opinion this is a
good version of MERGE and that is why I committed it. If it were not,
I would have no hesitation in pushing back a year or more, if I knew
of technical reasons to do so.

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

Hi,

On 2018-04-04 21:07:25 +0100, Simon Riggs wrote:
> It's also neat and tight. Look how easy it was for Peter to add WITH
> semantics on top of it.

Err. Several parts of the code definitely do not look "neat and
tight". As detailed in my email. Possibly that's necessary, but you've
not argued that.


> And it's isolated, so its not a threat to anybody that doesn't choose
> to use it. Users want it and will use this; if I didn't know that for
> certain I wouldn't spend time on it.

Architectural costs are a thing.


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


> If saying "I'm unhappy with something" is sufficient grounds for
> rejecting a patch, I'm surprised to hear it. There has been no
> discussion of what exactly would be better, only that what we have is
> somehow wrong, a point which both Pavan and I dispute, not least
> because the executor has already been rewritten once at Peter's
> request.

You've not publicly disputed that, no.


> I was under no pressure at all to commit this. In my opinion this is a
> good version of MERGE and that is why I committed it. If it were not,

Why did you then commit a patch six hours after objections were raised?
Without responding to them? And again breaking the patch into commits in
a way that made no sense and in fact was not compilable for an hour?

That does looks rushed, unless you provide a better explanation

- Andres

On Wed, Apr 4, 2018 at 1:07 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> This version works, with agreed semantics, all fully tested and documented.

I agree that it's more or less true that this works, and implements
the agreed-upon semantics. I also agree that that's very important.
That's beside the point, though.

> And it's isolated, so its not a threat to anybody that doesn't choose
> to use it. Users want it and will use this; if I didn't know that for
> certain I wouldn't spend time on it.

I strongly doubt it.

> If saying "I'm unhappy with something" is sufficient grounds for
> rejecting a patch, I'm surprised to hear it. There has been no
> discussion of what exactly would be better, only that what we have is
> somehow wrong, a point which both Pavan and I dispute, not least
> because the executor has already been rewritten once at Peter's
> request.

That's a total exaggeration. What happened was that Pavan cleaned up a
lot of the EPQ code, and related code in nodeModifyTable.c, as part of
getting the RC mode conflict handling right. Again, yes, that was
really essentially work.

A lot of the things that are bad about this patch are the same things
that were bad about my own ON CONFLICT patch before Andres arrived on
the scene. Very little changed about the fundamental semantics after
he joined that project, but a lot changed about the representation
used within the parser, planner, and executor. I think that the same
thing needs to happen here.

I knew from direct experience that it would be perfectly possible to
have a very useful discussion about the most important issue, the
semantics, without really having to address the very real concerns
that I had about the representation until a later date. Indeed, we
managed to do that, and I'm very glad that we managed to do that. It
was almost certainly the best strategy available.

Perhaps I should have been more forceful about the fundamental issue,
rather than making specific points about specific consequence of that
problem, but it probably wouldn't have made a big difference in the
end. There is only so much time available.

-- 
Peter Geoghegan

On 4 April 2018 at 21:14, Andres Freund <andres@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.

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

On Wed, Apr 04, 2018 at 10:10:46AM -0700, Andres Freund wrote:
> This needs at the very least a response to the issues pointed out in the
> referenced email that you chose to ignore without any sort of comment.

That's definitely not cool.
--
Michael

On Wed, Apr 04, 2018 at 10:10:46AM -0700, Andres Freund wrote:
> This needs at the very least a response to the issues pointed out in the
> referenced email that you chose to ignore without any sort of comment.

That's definitely not cool.
--
Michael

On Wed, Apr 4, 2018 at 3:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Well, what's on the table is reverting this patch and asking you to try
> again in the v12 cycle.  Given Andres' concerns about the executor design,
> and mine about the way the parsing end is built, there's certainly no way
> that that's all getting fixed by Saturday.  Given pretty much everybody's
> unhappiness with the way this patch was forced through at the last minute,
> I do not think you should expect that we'll say, "okay, we'll let you ship
> a bad version of MERGE because there's no more time in this cycle".

+1.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 4 April 2018 at 21:28, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 4 April 2018 at 21:14, Andres Freund <andres@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 Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-04-05 06:09:31 -0400, Robert Haas wrote:
> On Wed, Apr 4, 2018 at 3:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Well, what's on the table is reverting this patch and asking you to try
> > again in the v12 cycle.  Given Andres' concerns about the executor design,
> > and mine about the way the parsing end is built, there's certainly no way
> > that that's all getting fixed by Saturday.  Given pretty much everybody's
> > unhappiness with the way this patch was forced through at the last minute,
> > I do not think you should expect that we'll say, "okay, we'll let you ship
> > a bad version of MERGE because there's no more time in this cycle".
> 
> +1.

+1.

On Thu, Apr  5, 2018 at 11:15:20AM +0100, Simon Riggs wrote:
> On 4 April 2018 at 21:28, Simon Riggs <simon@2ndquadrant.com> wrote:
> > On 4 April 2018 at 21:14, Andres Freund <andres@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?

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Thu, Apr 05, 2018 at 04:02:20PM -0400, Bruce Momjian wrote:
> 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?

If my opinion counts, please count me in this bucket as well.  I have
seen also Peter G. commenting about the design of the patch in a very
advanced way and emit doubts, this is enough to convince me that
something wrong is going on here.  I have to admit that I did not look
at the patch in details but the design issues for the executor and
parser mentioned show that some low-level considerations have not been
taken into account, so this is worrying.
--
Michael

On 5 April 2018 at 21:02, Bruce Momjian <bruce@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 <simon@2ndquadrant.com> wrote:
>> > On 4 April 2018 at 21:14, Andres Freund <andres@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

On Thu, Apr 5, 2018 at 10:54 PM, Michael Paquier <michael@paquier.xyz> wrote:
> On Thu, Apr 05, 2018 at 04:02:20PM -0400, Bruce Momjian wrote:
>> 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?
>
> If my opinion counts, please count me in this bucket as well.  I have
> seen also Peter G. commenting about the design of the patch in a very
> advanced way and emit doubts, this is enough to convince me that
> something wrong is going on here.  I have to admit that I did not look
> at the patch in details but the design issues for the executor and
> parser mentioned show that some low-level considerations have not been
> taken into account, so this is worrying.

Apologies for butting in here as it is not my place. Just a rather
timid introduction, I have met most of you and am a huge fan all of
you. I have been reading hackers for many years. I will follow up with
a response in a vein similar to Michael's,  if my opinion counts,
which it probably does not, since this is my first post to hackers
ever:

 I have read the thread from the start and I don't think this is a
fair characterization of Peter's feedback. I would say initially yes
that would be a fair statement. However, in past 4-6 weeks I
interpreted his feedback as supportive. FWIW, I haven't read the patch
either and it would be of little value if I did :-).

Pavan did respond to all Peter's issues and implement all Peter's
requested changes and at one point spent a lot of time looking at and
reporting back on how another database handle certain situations with
MERGE so he could incorporate the proper behavior into Postgres and
properly respond to Peter's concerns. The community at large
requirements that MERGE support RLS and Partitioning were implemented
and Steven Frost reviewed the RLS implementation. The sqlsmith team
did extensive testing of the patch.

So given all this, I am not sure why people feel this patch was rushed
through or has a flawed design.  The comments from Andres while I am
sure they have merit came before the commit but technically after the
time when Simon said he was going to commit the patch (which he gave
with 5 days notice). The patch was developed and reviewed in the
community for many months. Pavan and Simon continue to respond on
these comments and implementing changes people are requesting.







> --
> Michael



-- 
Thomas John Kincaid

On Fri, Apr  6, 2018 at 09:21:54AM +0100, Simon Riggs wrote:
> On 5 April 2018 at 21:02, Bruce Momjian <bruce@momjian.us> wrote:
> > 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.

My point was that people didn't ask you to work harder on fixing the
patch, but in reverting it.  You can work harder on fixing things in the
hope they change their minds, but again, that isn't addressing their
request.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

Hi,

On 2018-04-06 12:22:09 -0400, Bruce Momjian wrote:
> On Fri, Apr  6, 2018 at 09:21:54AM +0100, Simon Riggs wrote:
> > On 5 April 2018 at 21:02, Bruce Momjian <bruce@momjian.us> wrote:
> > > 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.
> 
> My point was that people didn't ask you to work harder on fixing the
> patch, but in reverting it.  You can work harder on fixing things in the
> hope they change their minds, but again, that isn't addressing their
> request.

I concur with this description.  And you (Simon) have not at al reacted
to those points, instead very selectively quoting other parts of
relevant emails and responding to those.



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

I started looking at the patch because I became suspicious by the way
this was committed the first time round.  It was a brief skim of the
patch, so I could gauge the situation accurately. The fact that you
could commit any time of it was obviously a factor, so I obviously
didn't have time (nor responsibility!) to have a detailed description of
how it should like.  You then went ahead six hours later, committing it,
without even referring, not to mention responding, to my points.

There were several obviously things wrong in the patch. As the extreme
example, a non-executor node file was named as an executor node. That's
hardly something hidden in a corner, you've seperately committed that
change twice.  The parse structure issue was also fairly clear.  You
can't say that those aren't things one couldn't have found in careful
pre-commit review.  That, and the fact the commit was broken in a broken
(and non-compiling the second time round) way twice, does make this
appear rushed.


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

Nor does a single committer overrule three other committers raising
doubts. Without even xresponding to their concerns.


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

The patch looked *quite* different in January, so even if that were
true, what are you proposing that people do? Constantly look at features
that you say you'll commit in a few months? The state back then was
definitely not committable, so I don't even know what to make of this.

Greetings,

Andres Freund

On 2018-04-06 20:39:26 +0300, Alexander Korotkov wrote:
> On Fri, Apr 6, 2018 at 7:56 PM, Andres Freund <andres@anarazel.de> wrote:
> 
> > On 2018-04-06 12:22:09 -0400, Bruce Momjian wrote:
> > > 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.
> >
> > Nor does a single committer overrule three other committers raising
> > doubts. Without even xresponding to their concerns.
> >
> 
> BTW, what had happend to the idea of RMT voting for reverting patches?

My understanding is that the RMT managed period only really starts after
the 7th, once feature frozen.  And I think it's highly desirable to
attempt resolving disputes without resorting to such force, if possible.

Greetings,

Andres Freund

On Fri, Apr 6, 2018 at 1:21 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> 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.

Again, nobody disputed that.

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

Again, I don't think that the user visible-semantics are where we have
a problem here. It's an architectural problem.

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

I was feeling pretty burnt out towards the end, to be honest. Also,
the controversy surrounding this patch was discouraging to me
personally.

When I look at the patch, I understand why it evolved in the way it
evolved. The representation used in the parser is logical, in the
sense that it's the path of least resistance to get MERGE working. I
made similar mistakes myself with ON CONFLICT. That really wasn't that
much of a problem, though, because I admitted it.

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

A lot of the functionality that was added to MERGE in the past few
months were things that you were originally adamant were not within
scope for v11. In some cases, these were addressed with only modest
effort. It's pretty obvious to me that some aspects of MERGE's
architecture are accidents. Not necessarily happy accidents.

-- 
Peter Geoghegan

On Fri, Apr 6, 2018 at 7:37 AM, Tom Kincaid <tomjohnkincaid@gmail.com> wrote:
> So given all this, I am not sure why people feel this patch was rushed
> through or has a flawed design.  The comments from Andres while I am
> sure they have merit came before the commit but technically after the
> time when Simon said he was going to commit the patch (which he gave
> with 5 days notice). The patch was developed and reviewed in the
> community for many months. Pavan and Simon continue to respond on
> these comments and implementing changes people are requesting.

Simon attempted to present the initial version of this patch as more
or less finished, a point of view which others clearly did not share.
Over and over, people have raised concerns about things that the patch
should've handled and didn't.  Lots of people, perhaps Peter foremost
among them, have expended lots of emails trying to get the most
obvious problems with this patch fixed. Eventually, we all get tired
of arguing.  The burden of proof should be on Simon to show that this
patch is ready to commit and not on everyone else to show that isn't.
There are still issues that have been raised and not addressed, and
yet the patch was committed anyway -- in pieces -- and the reverted,
and then committed again -- still in pieces -- and then almost
immediately subject to a long series of follow-up commits fixing
various things -- but not all of the things that had been complained
about.  Those things should've been fixed before the patch was
committed.  Those things should have been fixed before Simon made a
statement of intention to commit.  The things that haven't been fixed
yet should've been fixed then, too.  And then the whole thing should
have gone in as one finished, completely-working feature in one
commit.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 6 April 2018 at 17:22, Bruce Momjian <bruce@momjian.us> wrote:
> On Fri, Apr  6, 2018 at 09:21:54AM +0100, Simon Riggs wrote:
>> On 5 April 2018 at 21:02, Bruce Momjian <bruce@momjian.us> wrote:
>> > 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.
>
> My point was that people didn't ask you to work harder on fixing the
> patch, but in reverting it.  You can work harder on fixing things in the
> hope they change their minds, but again, that isn't addressing their
> request.

I had understood Tom's post to be raising a discussion about whether to revert.

In my understanding, Tom's complaints were addressed quickly, against
what he expected - I was surprised myself at how quickly Pavan was
able to address them.

That left Andres' complaints, which as I explained hadn't been given
in any detail. Once given, Pavan investigated Andres' complaints and
explained in detail the results of his work and that he doesn't see
how the proposed changes would improve anything.

If there was anything unresolvable before commit I wouldn't have
committed it, or unresolvable after commit I would already have
reverted it.

And as I explained, this commit was not rushed and various other
negative points made are unfortunately not correct.

Now I can see some people are annoyed, so I'm happy to apologize if
I've done things that weren't understood or caused annoyance. Time is
short at end of CF and tempers fray for us all.

If Tom or Andres still feel that their concerns have not been
addressed over the last few days, I am happy to revert the patch with
no further discussion from me in this cycle.

If request to revert occurs, I propose to do this on Wed 11 pm, to
avoid getting in the way of other commits and because it will take
some time to prepare to revert.

Thanks to everyone for review comments and well done to Pavan,
whatever we decide.

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

On Sat, Apr  7, 2018 at 06:19:19PM +0100, Simon Riggs wrote:
> Now I can see some people are annoyed, so I'm happy to apologize if
> I've done things that weren't understood or caused annoyance. Time is
> short at end of CF and tempers fray for us all.
> 
> If Tom or Andres still feel that their concerns have not been
> addressed over the last few days, I am happy to revert the patch with
> no further discussion from me in this cycle.

Yep, I think the right thing, as you have just done, is to ask for clear
confirmation on revert, and accept whatever people say.  I would revert
if anyone requests it.

To summarize how I see this patch, we have this workflow at the top of
the TODO list (which I think Simon helped with or suggested):

    Desirability -> Design -> Implement -> Test -> Review -> Commit

I think the MERGE patch spent a long time getting through the first and
second stages, and now the objections appear to be related to
implementation.  I think the implementation issues only appeared during the
final commitfest, which made it feel like a new patch.  Yes, it had been
through the first two stages before the final commitfest.

I think one of the missing rules we have is that when we say no new
large patches in the final commitfest, do we mean that all _three_
stages should be solidified before the final commitfest?  I have never
been clear on that point.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

Simon Riggs <simon@2ndquadrant.com> writes:
> On 6 April 2018 at 17:22, Bruce Momjian <bruce@momjian.us> wrote:
>> My point was that people didn't ask you to work harder on fixing the
>> patch, but in reverting it.  You can work harder on fixing things in the
>> hope they change their minds, but again, that isn't addressing their
>> request.

> If Tom or Andres still feel that their concerns have not been
> addressed over the last few days, I am happy to revert the patch with
> no further discussion from me in this cycle.

FWIW, I still vote to revert.  Even if the patch were now perfect,
there is not time for people to satisfy themselves of that, and
we've got lots of other things on our plates.

I'd be glad to participate in a proper review of this when v12
opens.  But right now it just seems too rushed, and I have little
confidence in it being right.

            regards, tom lane

PS: If you do revert, please wrap it up as a single revert commit,
not a series of half a dozen.  You've already put several
non-buildable states into the commit history as a result of this
patch, each one of which is a land mine for git bisect testing.
We don't need more of those.  Also, looking at the reverse of the
reversion commit will provide a handy way of seeing the starting
point for future discussion of this patch.

Hi,

On 2018-04-07 13:45:21 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:
> > On 6 April 2018 at 17:22, Bruce Momjian <bruce@momjian.us> wrote:
> >> My point was that people didn't ask you to work harder on fixing the
> >> patch, but in reverting it.  You can work harder on fixing things in the
> >> hope they change their minds, but again, that isn't addressing their
> >> request.
> 
> > If Tom or Andres still feel that their concerns have not been
> > addressed over the last few days, I am happy to revert the patch with
> > no further discussion from me in this cycle.
> 
> FWIW, I still vote to revert.  Even if the patch were now perfect,
> there is not time for people to satisfy themselves of that, and
> we've got lots of other things on our plates.

Precisely.

And no, I don't think the concerns have been addressed fully.


> I'd be glad to participate in a proper review of this when v12
> opens.  But right now it just seems too rushed, and I have little
> confidence in it being right.

Yea, I'd really hope this gets submitted *early* in the v12 cycle rather
than leaving it to the end.  I'd even be willing to produce a prototype
of what I think should be changed in the parse-analysis & executor
stages, if necessary.

Greetings,

Andres Freund

Hi,

On 2018-04-07 13:33:53 -0400, Bruce Momjian wrote:
> To summarize how I see this patch, we have this workflow at the top of
> the TODO list (which I think Simon helped with or suggested):
> 
>     Desirability -> Design -> Implement -> Test -> Review -> Commit
> 
> I think the MERGE patch spent a long time getting through the first and
> second stages

The current implementation effort publicly started 2017-10-27. For a
major feature that's really not that long ago. There also were a few
gaps in which no public development/discussion happened.


>, and now the objections appear to be related to implementation.  I
>think the implementation issues only appeared during the final
>commitfest, which made it feel like a new patch.  Yes, it had been
>through the first two stages before the final commitfest.

I'm not sure there was agreement on the design even. A lot of that has
been discussed until very recently.


> I think one of the missing rules we have is that when we say no new
> large patches in the final commitfest, do we mean that all _three_
> stages should be solidified before the final commitfest?  I have never
> been clear on that point.

I think the implementation at the least should be roughly ready, and
implement a roughly agreed upon design. It's fine to change things
around, but major re-engineering surely is an alarm sign.

Greetings,

Andres Freund

On 7 April 2018 at 18:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:
>> On 6 April 2018 at 17:22, Bruce Momjian <bruce@momjian.us> wrote:
>>> My point was that people didn't ask you to work harder on fixing the
>>> patch, but in reverting it.  You can work harder on fixing things in the
>>> hope they change their minds, but again, that isn't addressing their
>>> request.
>
>> If Tom or Andres still feel that their concerns have not been
>> addressed over the last few days, I am happy to revert the patch with
>> no further discussion from me in this cycle.
>
> FWIW, I still vote to revert.  Even if the patch were now perfect,
> there is not time for people to satisfy themselves of that, and
> we've got lots of other things on our plates.
>
> I'd be glad to participate in a proper review of this when v12
> opens.  But right now it just seems too rushed, and I have little
> confidence in it being right.
>
>                         regards, tom lane
>
> PS: If you do revert, please wrap it up as a single revert commit,
> not a series of half a dozen.  You've already put several
> non-buildable states into the commit history as a result of this
> patch, each one of which is a land mine for git bisect testing.
> We don't need more of those.  Also, looking at the reverse of the
> reversion commit will provide a handy way of seeing the starting
> point for future discussion of this patch.

Will do.



"Commence primary ignition."

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

On 11 April 2018 at 21:00, Simon Riggs <simon@2ndquadrant.com> wrote:

> "Commence primary ignition."

Revert patch attached

Mostly Pavan, minor cleanup by me


Requesting review so I don't cause multiple revert commits.

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

On Wed, Apr 11, 2018 at 10:28:10PM +0100, Simon Riggs wrote:
> Requesting review so I don't cause multiple revert commits.

I have gone through the code tree with your patch applied for some time,
and it seems to me that there are no more traces of merge or any of its
infrastructure.  Tests are also able to pass.
--
Michael

On 4/11/18 5:28 PM, Simon Riggs wrote:
> On 11 April 2018 at 21:00, Simon Riggs <simon@2ndquadrant.com> wrote:
> 
>> "Commence primary ignition."
> 
> Revert patch attached
> 
> Mostly Pavan, minor cleanup by me

I have moved this entry to the next CF in Waiting for Author state.

Regards,
-- 
-David
david@pgmasters.net