On Fri, Apr 6, 2018 at 1:30 AM, Andres Freund <andres@anarazel.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.
But wouldn't this lead to lot of code duplication? For example, ExecInsert/ExecUpdate does a bunch of supporting work (firing triggers, inserting into indexes just to name a few) that MERGE's INSERT/UPDATE needs as well. Now we can possibly move these support routines to a new file, say execModify.c and then let both Merge as well as ModifyTable node make use of that. But the fact is that ExecInsert/ExecUpdate knows a lot about ModifyTable already. So to separate ExecInsert/ExecUpdate from ModifyTable will require significant refactoring AFAICS. I am not saying we can't do that, but will have it's own consequences.
If we would not have refactored to move ExecMerge and friends to a new file, then it may not have looked so odd (as you describe above). But I think moving the code to a new file was a net improvement. May be we can move ExecInsert/Update etc to a new file as I suggested, but still use the ModifyTable to run Merge. There are many things common between them. ModifyTable executes all DMLs and MERGE is just another DML which can run all three.
