Re: [HACKERS] MERGE SQL Statement for PG11

On 2018/03/23 13:57, Pavan Deolasee wrote:
> On Fri, Mar 23, 2018 at 10:00 AM, Amit Langote wrote:
>> I managed to apply it by ignoring the errors, but couldn't get make check
>> to pass; attached regressions.diffs if you want to take a look.
>
> Thanks. Are you sure you're using a clean repo? I suspect you'd a previous
> version of the patch applied and hence the apply errors now. I also suspect
> that you may have made a mistake while resolving the conflicts while
> applying the patch (since a file at the same path existed). The failures
> also seem related to past version of the patch.
> 
> I just checked with a freshly checked out repo and the patches apply
> correctly on the current master and regression passes too.
> http://commitfest.cputube.org/ also reported success overnight.

You're right, I seem to have messed something up.  Sorry about the noise.

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.

Sorry that this is me coming a bit late to this thread, but I noticed a
few things in patch that I thought I should comment on.

1. White space errors

$ git diff master --check
src/backend/executor/execPartition.c:737: trailing whitespace.
+        /*
src/backend/executor/nodeMerge.c:90: indent with spaces.
+        PartitionTupleRouting *proute = mtstate->mt_partition_tuple_routing;
src/backend/executor/nodeMerge.c:116: trailing whitespace.
+
src/backend/executor/nodeMerge.c:565: new blank line at EOF.

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.

Thanks,
Amit