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