Re: [HACKERS] MERGE SQL Statement for PG11

On Mon, Apr 2, 2018 at 7:18 PM, Andres Freund <andres(at)anarazel(dot)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