Re: [HACKERS] MERGE SQL Statement for PG11

@@ -3828,8 +3846,14 @@ struct AfterTriggersTableData
    bool        before_trig_done;   /* did we already queue BS triggers? */
    bool        after_trig_done;    /* did we already queue AS triggers? */
    AfterTriggerEventList after_trig_events;    /* if so, saved list pointer */
-   Tuplestorestate *old_tuplestore;    /* "old" transition table, if any */
-   Tuplestorestate *new_tuplestore;    /* "new" transition table, if any */
+   /* "old" transition table for UPDATE, if any */
+   Tuplestorestate *old_upd_tuplestore;
+   /* "new" transition table for UPDATE, if any */
+   Tuplestorestate *new_upd_tuplestore;
+   /* "old" transition table for DELETE, if any */
+   Tuplestorestate *old_del_tuplestore;
+   /* "new" transition table INSERT, if any */
+   Tuplestorestate *new_ins_tuplestore;
 };

A comment somewhere why we have all of these (presumably because they
can now happen in the context of a single statement) would be good.

@@ -5744,12 +5796,28 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
                 newtup == NULL));

        if (oldtup != NULL &&
-           ((event == TRIGGER_EVENT_DELETE && delete_old_table) ||
-            (event == TRIGGER_EVENT_UPDATE && update_old_table)))
+           (event == TRIGGER_EVENT_DELETE && delete_old_table))
        {
            Tuplestorestate *old_tuplestore;

-           old_tuplestore = transition_capture->tcs_private->old_tuplestore;
+           old_tuplestore = transition_capture->tcs_private->old_del_tuplestore;
+
+           if (map != NULL)
+           {
+               HeapTuple   converted = do_convert_tuple(oldtup, map);
+
+               tuplestore_puttuple(old_tuplestore, converted);
+               pfree(converted);
+           }
+           else
+               tuplestore_puttuple(old_tuplestore, oldtup);
+       }

Very similar code is now repeated four times. Could you abstract this
into a separate function?

--- a/src/backend/executor/README
+++ b/src/backend/executor/README
@@ -37,6 +37,16 @@ the plan tree returns the computed tuples to be updated, plus a "junk"
 one.  For DELETE, the plan tree need only deliver a CTID column, and the
 ModifyTable node visits each of those rows and marks the row deleted.

+MERGE runs one generic plan that returns candidate target rows. Each row
+consists of a super-row that contains all the columns needed by any of the
+individual actions, plus a CTID and a TABLEOID junk columns. The CTID column is

"plus a CTID and a TABLEOID junk columns" sounds a bit awkward?

+required to know if a matching target row was found or not and the TABLEOID
+column is needed to find the underlying target partition, in case when the
+target table is a partition table. If the CTID column is set we attempt to
+activate WHEN MATCHED actions, or if it is NULL then we will attempt to

"if it is NULL then" sounds wrong.

+activate WHEN NOT MATCHED actions. Once we know which action is activated we
+form the final result row and apply only those changes.
+
 XXX a great deal more documentation needs to be written here...

Are we using tableoids in a similar way in other places?

+/*
+ * Given OID of the partition leaf, return the index of the leaf in the
+ * partition hierarchy.
+ */
+int
+ExecFindPartitionByOid(PartitionTupleRouting *proute, Oid partoid)
+{
+   int i;
+
+   for (i = 0; i < proute->num_partitions; i++)
+   {
+       if (proute->partition_oids[i] == partoid)
+           break;
+   }
+
+   Assert(i < proute->num_partitions);
+   return i;
+}

Shouldn't we at least warn in a comment that this is O(N)? And document
that it does weird stuff if the OID isn't found?

Perhaps just introduce a PARTOID syscache?

diff --git a/src/backend/executor/nodeMerge.c b/src/backend/executor/nodeMerge.c
new file mode 100644
index 00000000000..0e0d0795d4d
--- /dev/null
+++ b/src/backend/executor/nodeMerge.c
@@ -0,0 +1,575 @@
+/*-------------------------------------------------------------------------
+ *
+ * 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.

Might also be worthwhile to move the merge related init stuff here?

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

FWIW, I'd re-order this file so this routine is above
ExecMergeMatched(), ExecMergeNotMatched(), easier to understand.

+   /*
+    * If there are not WHEN MATCHED actions, we are done.
+    */
+   if (mergeMatchedActionStates == NIL)
+       return true;

Maybe I'm confused, but why is mergeMatchedActionStates attached to
per-partition info? How can this differ in number between partitions,
requiring us to re-check it below fetching the partition info above?

+       /*
+        * Check for any concurrent update/delete operation which may have
+        * prevented our update/delete. We also check for situations where we
+        * might be trying to update/delete the same tuple twice.
+        */
+       if ((action->commandType == CMD_UPDATE && !tuple_updated) ||
+           (action->commandType == CMD_DELETE && !tuple_deleted))
+
+       {
+           switch (hufd.result)
+           {
+               case HeapTupleMayBeUpdated:
+                   break;
+               case HeapTupleInvisible:
+
+                   /*
+                    * This state should never be reached since the underlying
+                    * JOIN runs with a MVCC snapshot and should only return
+                    * rows visible to us.
+                    */

Given EPQ, that reasoning isn't correct. I think this should still be
unreachable, just not for the reason described here.

It seems fairly bad architecturally to me that we now have
EvalPlanQual() loops in both this routine *and*
ExecUpdate()/ExecDelete().

+
+/*
+ * Execute the first qualifying NOT MATCHED action.
+ */
+static void
+ExecMergeNotMatched(ModifyTableState *mtstate, EState *estate,
+                   TupleTableSlot *slot)
+{
+   PartitionTupleRouting *proute = mtstate->mt_partition_tuple_routing;
+   ExprContext *econtext = mtstate->ps.ps_ExprContext;
+   List       *mergeNotMatchedActionStates = NIL;
+   ResultRelInfo *resultRelInfo;
+   ListCell   *l;
+   TupleTableSlot  *myslot;
+
+   /*
+    * We are dealing with NOT MATCHED tuple. Since for MERGE, partition tree

*the partition tree

+    * is not expanded for the result relation, we continue to work with the
+    * currently active result relation, which should be of the root of the
+    * partition tree.
+    */
+   resultRelInfo = mtstate->resultRelInfo;

"should be"? "is", I hope?  Given that it's referencing
mtstate->resultRelInfo which is only set in one place...

+/* flags for mt_merge_subcommands */
+#define MERGE_INSERT   0x01
+#define MERGE_UPDATE   0x02
+#define MERGE_DELETE   0x04

Hm, it seems a bit weird to define these file-local, given there's
another file implementing a good chunk of merge.

+   /*
+    * Initialize hufdp. Since the caller is only interested in the failure
+    * status, initialize with the state that is used to indicate successful
+    * operation.
+    */
+   if (hufdp)
+       hufdp->result = HeapTupleMayBeUpdated;
+

This signals for me that the addition of the result field to HUFD wasn't
architecturally the right thing. HUFD is supposed to be supposed to be
returned by heap_update(), reusing and setting it from other places
seems like a layering violation to me.

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 69dd327f0c9..cd540a0df5b 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -851,6 +851,60 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
                        fix_scan_list(root, splan->exclRelTlist, rtoffset);
                }

+               /*
+                * The MERGE produces the target rows by performing a right

s/the MERGE/the MERGE statement/?

+                * join between the target relation and the source relation
+                * (which could be a plain relation or a subquery). The INSERT
+                * and UPDATE actions of the MERGE requires access to the

same.

+opt_and_condition:

Renaming this to be a bit more merge specific seems like a good idea.

+
+   /*
+    * Add a whole-row-Var entry to support references to "source.*".
+    */
+   var = makeWholeRowVar(rt_rte, rt_rtindex, 0, false);
+   te = makeTargetEntry((Expr *) var, list_length(*mergeSourceTargetList) + 1,
+                        NULL, true);

Why can't this properly dealt with by transformWholeRowRef() etc?

+                   if (selectStmt && (selectStmt->valuesLists == NIL ||
+                                      selectStmt->sortClause != NIL ||
+                                      selectStmt->limitOffset != NULL ||
+                                      selectStmt->limitCount != NULL ||
+                                      selectStmt->lockingClause != NIL ||
+                                      selectStmt->withClause != NULL))
+                       ereport(ERROR,
+                               (errcode(ERRCODE_SYNTAX_ERROR),
+                                errmsg("SELECT not allowed in MERGE INSERT statement")));

+                   if (selectStmt && list_length(selectStmt->valuesLists) > 1)
+                       ereport(ERROR,
+                               (errcode(ERRCODE_SYNTAX_ERROR),
+                                errmsg("Multiple VALUES clauses not allowed in MERGE INSERT statement")));

Shouldn't this include an error position?

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.

+   /*
+    * XXX MERGE is unsupported in various cases
+    */
+   if (!(pstate->p_target_relation->rd_rel->relkind == RELKIND_RELATION ||
+         pstate->p_target_relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
+       ereport(ERROR,
+               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                errmsg("MERGE is not supported for this relation type")));

Shouldn't this report what relation type the error talking about? It's
not going to necessarily be obvious to the user.  Also, errposition etc
would be good.

+                       /*
+                        * Do basic expression transformation (same as a ROW()
+                        * expr, but allow SetToDefault at top level)
+                        */
+                       exprList = transformExpressionList(pstate,
+                                                          (List *) linitial(valuesLists),
+                                                          EXPR_KIND_VALUES_SINGLE,
+                                                          true);
+
+                       /* Prepare row for assignment to target table */
+                       exprList = transformInsertRow(pstate, exprList,
+                                                     istmt->cols,
+                                                     icolumns, attrnos,
+                                                     false);
+                   }

Can't we handle this with a littlebit less code duplication?

 typedef struct HeapUpdateFailureData
 {
+   HTSU_Result result;
    ItemPointerData ctid;
    TransactionId xmax;
    CommandId   cmax;
+   LockTupleMode   lockmode;
 } HeapUpdateFailureData;


These new fields seem not really relateto HUFD, but rather just fields
the merge code should maintain?