Re: MERGE SQL statement for PG12

Hi Tomas,

Sorry for a delayed response.

On Mon, Oct 29, 2018 at 4:59 PM Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
wrote:

> Hi Pavan,
>
> On 10/29/2018 10:23 AM, Pavan Deolasee wrote:
> >
> > ...
> >
> > Thanks for keeping an eye on the patch. I've rebased the patch
> > against the current master. A new version is attached.
> >
> > Thanks,
> > Pavan
> >
>
> It's good to see the patch moving forward. What are your thoughts
> regarding things that need to be improved/fixed to make it committable?
>
> I briefly discussed the patch with a couple of people at pgconf.eu last
> week, and IIRC the main concern was how the merge is represented in
> parser/executor.
>
>
Yes, Andres and to some extent Peter G had expressed concerns about that.
Andres suggested that we should rework the parser and executor side. Here
are some excerpts from his review comments.

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

+ * As top-level statements INSERT, UPDATE and DELETE have a Query,
whereas
+ * with MERGE the individual actions do not require separate planning,
+ * only different handling in the executor. See nodeModifyTable handling
+ * of commandType CMD_MERGE.
+ *
+ * A sub-query can include the Target, but otherwise the sub-query
cannot
+ * reference the outermost Target table at all.
+ */
+ qry->querySource = QSRC_PARSER;

Why is this, and not building a proper executor node for merge that
knows how to get the tuples, the right approach? We did a rough
equivalent for matview updates, and I think it turned out to be a pretty
bad plan.
</review>

<review>
On Fri, Apr 6, 2018 at 1:30 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:

>
>
> My impression is that this simply shouldn't be going through
> nodeModifyTuple, but be it's own nodeMerge node. The trigger handling
> would need to be abstraced into execTrigger.c or such to avoid
> duplication. We're now going from nodeModifyTable.c:ExecModifyTable()
> into execMerge.c:ExecMerge(), back to
> nodeModifyTable.c:ExecUpdate/Insert(). To avoid ExecInsert() doing
> things that aren't appropriate for merge, we then pass an actionState,
> which neuters part of ExecUpdate/Insert(). This is just bad.
>

</review>

To be honest, I need more directions on how to address these major
architectural concerns. I'd tried to rework the executor part and had
commented on that. But I know that was probably done in a hurry since we
were trying to save a revert. Having said that, I am still not very sure
how exactly the executor side should look like without causing too much
duplication of handling nodeModifyTable() and proposed nodeMerge(). If
Andres can give me further inputs, I will be more than happy to figure out
the details and improve the patch.

As far as the parser side goes, do I understand correctly that instead of
building a special join in transformMergeStmt() as the patch does today, we
should follow what transformUpdateStmt() does for a potential join? i.e.
just have a single RTE for the source relation and present it as a left
hand side for the implicit join? If I get a little more direction on this,
I can try to address the issues.

It looks very likely that the patch won't get much review in the current
state. But if I get inputs, I can work out the details and try to get the
patch in committable state.

BTW I am aware that the patch is bit-rotten because of the partitioning
related changes. I will address those and post a revised patch soon.

Thanks,
Pavan

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