Re: [HACKERS] MERGE SQL Statement for PG11 - Mailing list pgsql-hackers

Also, it seems that the delta patch I sent in the last email didn't
contain all the changes I had to make.  It didn't contain, for example,
replacing adjust_and_expand_inherited_tlist() with
adjust_partition_tlist().  I guess you'll know when you rebase anyway.

1. White space errors

$ git diff master --check

2. Sorry if this has been discussed before, but is it OK to use AclMode
like this:

+
+    AclMode     mt_merge_subcommands;   /* Flags show which cmd types are
+                                         * present */

3. I think the comment above this should be updated to explain why the
map_index is being set to the leaf index in case of MERGE instead of the
subplan index as is done in case of plain UPDATE:

-            map_index = resultRelInfo - mtstate->resultRelInfo;
-            Assert(map_index >= 0 && map_index < mtstate->mt_nplans);
-            tupconv_map = tupconv_map_for_subplan(mtstate, map_index);
+            if (mtstate->operation == CMD_MERGE)
+            {
+                map_index = resultRelInfo->ri_PartitionLeafIndex;
+                Assert(mtstate->rootResultRelInfo == NULL);
+                tupconv_map = TupConvMapForLeaf(proute,
+                                mtstate->resultRelInfo,
+                                map_index);
+            }
+            else
+            {

4. Do you  think it would be possible at a later late to change this junk
attribute to contain something other than "tableoid"?

+                        if (operation == CMD_MERGE)
+                        {
+                            j->jf_otherJunkAttNo =
ExecFindJunkAttribute(j, "tableoid");
+                            if
(!AttributeNumberIsValid(j->jf_otherJunkAttNo))
+                                elog(ERROR, "could not find junk tableoid
column");
+
+                        }

Currently, it seems ExecMergeMatched will take this OID and look up the
partition (its ResultRelInfo) by calling ExecFindPartitionByOid which in
turn looks it up in the PartitionTupleRouting struct.  I'm imagining it
might be possible to instead return an integer that specifies "a partition
number".  Of course, nothing like that exists currently, but just curious
if we're going to be "stuck" with this junk attribute always containing
"tableoid".  Or maybe putting a "partition number" into the junk attribute
is not doable to begin with.

5. In ExecInitModifyTable, in the if (node->mergeActionList) block:

+
+        /* initialize slot for the existing tuple */
+        mtstate->mt_existing = ExecInitExtraTupleSlot(estate, NULL);

Maybe a good idea to Assert that mt_existing is NULL before.  Also, why
not set the slot's descriptor right away if tuple routing is not going to
be used.  I did it that way in the ON CONFLICT DO UPDATE patch.

6. I see that there is a slot called mergeSlot that becomes part of
ResultRelInfo of the table (partition) via ri_MergeState.  That means we
might end up creating as many slots as there are partitions (* number of
actions?).  Can't we have just one, say, mt_mergeproj in ModifyTableState
similar to mt_conflproj and just reset its descriptor before use.  I guess
reset will have happen before carrying out an action applied to a given
partition.  When I tried that (see attached delta), nothing got broken.