On Mon, Apr 2, 2018 at 7:18 PM, Andres Freund <andres@anarazel.de> wrote:
> I did a scan through this, as I hadn't been able to keep with the thread
> previously. Sorry if some of the things mentioned here have been
> discussed previously. I am just reading through the patch in its own
> order, so please excuse if there's things I remark on that only later
> fully make sense.
>
>
> later update: TL;DR: I don't think the parser / executor implementation
> of MERGE is architecturally sound. I think creating hidden joins during
> parse-analysis to implement MERGE is a seriously bad idea and it needs
> to be replaced by a different executor structure.
+1. I continue to have significant misgivings about this. It has many
consequences that we know about, and likely quite a few more that we
don't.
> So what happens if there's a concurrent insertion of a potentially
> matching tuple?
It's not a special case. In all likelihood, you get a dup violation.
This is a behavior that I argued for from an early stage.
> It seems fairly bad architecturally to me that we now have
> EvalPlanQual() loops in both this routine *and*
> ExecUpdate()/ExecDelete().
I think that that makes sense, because ExecMerge() doesn't use any of
the EPQ stuff from ExecUpdate() and ExecDelete(). It did at one point,
in fact, and it looked quite a lot worse IMV.
> *Gulp*. I'm extremely doubtful this is a sane approach. At the very
> least the amount of hackery required to make existing code cope with
> the approach / duplicate code seems indicative of that.
I made a lot of the fact that there are two RTEs for the target, since
that has fairly obvious consequences during every stage of query
processing. However, I think you're right that the problem is broader
than that.
> + if (pstate->p_target_relation->rd_rel->relhasrules)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("MERGE is not supported for relations with rules")));
>
> I guess we can add that later, but it's a bit sad that this won't work
> against views with INSTEAD OF triggers.
It also makes it hard to test deparsing support, which actually made
it in in the end. That must be untested.
--
Peter Geoghegan