From: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
---|---|
To: | Amit Langote <amitlangote09(at)gmail(dot)com> |
Cc: | Daniel Gustafsson <daniel(at)yesql(dot)se>, Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: partition routing layering in nodeModifyTable.c |
Date: | 2020-10-15 14:59:13 |
Message-ID: | cffe0dd6-8eba-4856-68cf-476fdb8a1235@iki.fi |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Oct 14, 2020 at 6:04 PM Heikki Linnakangas <hlinnaka(at)iki(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Bruce Momjian | 2020-10-15 15:14:15 | Re: Aw: Re: Minor documentation error regarding streaming replication protocol |
Previous Message | Bruno Lavoie | 2020-10-15 14:23:33 | Packaging - Packages names consistency (RPM) |