Re: [HACKERS] MERGE SQL Statement for PG11

On Fri, Mar 23, 2018 at 12:57 PM, Amit Langote <
Langote_Amit_f8@lab.ntt.co.jp> wrote:

>
> 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.
>

Yes, I am planning to fix that once the ON CONFLICT patch is
ready/committed.


>
> 1. White space errors
>
> $ git diff master --check
>
>
Fixed.


>
> 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 */
>

Hmm. I think you're right. Defined required flags in nodeModifyTable.c and
using those now.


>
> 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
> +            {
>

Done. I wonder though if we should just always set ri_PartitionLeafIndex
even for regular UPDATE and always use that to retrieve the map.


>
> 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.
>

I am not sure. Wouldn't adding a new junk column require a whole new
machinery? It might be worth adding it someday to reduce the cost
associated with the lookups. But I don't want to include the change in this
already largish patch.


>
> 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.
>

Yeah, I plan to update this code once the other patch gets in. This change
was mostly borrowed from your/Alvaro's patch, but I don't think it will be
part of the MERGE patch if the ON CONFLICT DO UPDATE patch gets in ahead of
this.


>
> 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! It was on my TODO list. So thanks for taking care of it. I've
included your patch in the main patch. I imagine we can similarly set the
tuple descriptor for this slot during initialisation if target table is a
non-partitioned table. But I shall take care of that along with
mt_existing. In fact, I wonder if we should combine mt_confproj and
mt_mergeproj and just have one slot. They are mutually exclusive in their
use, but have lot in common.

As someone who understands partitioning best, do you have any other
comments/concerns regarding partitioning related code in the patch? I would
appreciate if you can give it a look and also run any tests that you may
have handy.

Thanks,
Pavan

-- 
 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services