Re: partition routing layering in nodeModifyTable.c

On Wed, Oct 14, 2020 at 6:04 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> I'll continue with the last couple of patches in this thread.

I committed the move of the cross-partition logic to new 
ExecCrossPartitionUpdate() function, with just minor comment editing and 
pgindent. I left out the refactoring around the calls to AFTER ROW 
INSERT/DELETE triggers. I stared at the change for a while, and wasn't 
sure if I liked the patched or the unpatched new version better, so I 
left it alone.

Looking at the last patch, "Revise child-to-root tuple conversion map 
management", that's certainly an improvement. However, I find it 
confusing that sometimes the mapping from child to root is in 
relinfo->ri_ChildToRootMap, and sometimes in 
relinfo->ri_PartitionInfo->pi_PartitionToRootMap. When is each of those 
filled in? If both are set, is it well defined which one is initialized 
first?

In general, I'm pretty confused by the initialization of 
ri_PartitionInfo. Where is initialized, and when? In execnodes.h, the 
definition of ResultRelInfo says:

>     /* info for partition tuple routing (NULL if not set up yet) */
>     struct PartitionRoutingInfo *ri_PartitionInfo;

That implies that the field is initialized lazily. But in 
ExecFindPartition, we have this:

>         if (partidx == partdesc->boundinfo->default_index)
>         {
>             PartitionRoutingInfo *partrouteinfo = rri->ri_PartitionInfo;
> 
>             /*
>              * The tuple must match the partition's layout for the constraint
>              * expression to be evaluated successfully.  If the partition is
>              * sub-partitioned, that would already be the case due to the code
>              * above, but for a leaf partition the tuple still matches the
>              * parent's layout.
>              *
>              * Note that we have a map to convert from root to current
>              * partition, but not from immediate parent to current partition.
>              * So if we have to convert, do it from the root slot; if not, use
>              * the root slot as-is.
>              */
>             if (partrouteinfo)
>             {
>                 TupleConversionMap *map = partrouteinfo->pi_RootToPartitionMap;
> 
>                 if (map)
>                     slot = execute_attr_map_slot(map->attrMap, rootslot,
>                                                  partrouteinfo->pi_PartitionTupleSlot);
>                 else
>                     slot = rootslot;
>             }
> 
>             ExecPartitionCheck(rri, slot, estate, true);
>         }

That check implies that it's not just lazily initialized, the code will 
work differently if ri_PartitionInfo is set or not.

I think all this would be more clear if ri_PartitionInfo and 
ri_ChildToRootMap were both truly lazily initialized, the first time 
they're needed. And if we removed 
ri_PartitionInfo->pi_PartitionToRootMap, and always used 
ri_ChildToRootMap for it.

Maybe remove PartitionRoutingInfo struct altogether, and just move its 
fields directly to ResultRelInfo.

- Heikki