Re: [HACKERS] MERGE SQL Statement for PG11

Hi,

On 2018-04-05 11:31:48 +0530, Pavan Deolasee wrote:
> > +/*---------------------------------------------------------
> > ----------------
> > + *
> > + * nodeMerge.c
> > + *   routines to handle Merge nodes relating to the MERGE command
> >
> > Isn't this file misnamed and it should be execMerge.c?  The rest of the
> > node*.c files are for nodes that are invoked via execProcnode /
> > ExecProcNode().  This isn't an actual executor node.
> >
>
> Makes sense. Done. (Now that the patch is committed, I don't know if there
> would be a rethink about changing file names. May be not, just raising that
> concern)

It absolutely definitely needed to be renamed. But that's been done, so
...


> > What's the reasoning behind making this be an anomaluous type of
> > executor node?

> Didn't quite get that. I think naming of the file was bad (fixed now), but
> I think it's a good idea to move the new code in a new file, from
> maintainability as well as coverage perspective. If you've something very
> different in mind, can you explain in more details?

Well, it was kinda modeled as an executor node, including the file
name. That's somewhat fixed now.   I still am extremely suspicious of
the codeflow here.

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.