Re: [HACKERS] MERGE SQL Statement for PG11

On Fri, Mar 23, 2018 at 12:57 PM, Amit Langote <
Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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