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

On Wed, Apr 4, 2018 at 10:40 PM, Andres Freund <andres@anarazel.de> wrote:

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.

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.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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 Thu, Apr 5, 2018 at 12:16 AM, Andres Freund <andres@anarazel.de> wrote:

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.

While I don't want to answer that on Simon's behalf, my feeling is that he
may not seen your email since it came pretty late. He had probably planned
to commit the patch again first thing in the morning with the fixes I'd
sent.

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.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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