Subject: Re: partition routing layering in nodeModifyTable.c
To: Amit Langote <amitlangote09@gmail.com>
Cc: Daniel Gustafsson <daniel@yesql.se>,
 Etsuro Fujita <etsuro.fujita@gmail.com>, Andres Freund <andres@anarazel.de>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <20190718010911.l6xcdv6birtxiei4@alap3.anarazel.de>
 <73fe6ce6-d2a1-bd4b-17cf-9deba4e91467@iki.fi>
 <CA+HiwqGAVa9J9hNLH7tVCs13UqJMP-9RU_T5uAWEAfsg8VEXGQ@mail.gmail.com>
 <19c23dd9-89ce-75a3-9105-5fc05a46f94a@iki.fi>
 <CA+HiwqHtriD3HquH5=uT5caHd4jiEoxeb5JD2JSZRm8gDXsRxw@mail.gmail.com>
 <CA+HiwqGEmiib8FLiHMhKB+CH5dRgHSLc5N5wnvc4kym+ZYpQEQ@mail.gmail.com>
 <9bfb949e-21b6-e0f7-c9c9-58710d11e7b1@iki.fi>
 <CA+HiwqHX4gt6b7e=xM3aG837Hfv=q5hEkQ=E1BSi54ob=_cQaA@mail.gmail.com>
 <3603af69-eb52-4891-5d50-669cb03090a6@iki.fi>
 <CA+HiwqENbJMzoD1VAoNWFLcnam0zJS34Vy0o5AuhW-UVq1LV6A@mail.gmail.com>
 <ddae2239-4932-fb08-e501-9768cee871fc@iki.fi>
 <CA+HiwqGCiipPyFLSvO9K=xbT2yPb2EPN1OTi-YMBgOR7UEuaQA@mail.gmail.com>
 <a7e25ec8-4c46-1636-1a96-0c38fa5fde96@iki.fi>
 <be90708f-9717-fb37-9cc1-d2482f7e4230@iki.fi>
 <CA+HiwqEHechwotq=Lo0PRwZ7AP9fNQQnnJMYFYVDAOr09SrYKw@mail.gmail.com>
 <902a656d-e4f2-70bf-f30f-1796d65791fe@iki.fi>
 <CA+HiwqGYu4bkvFufvnW2Rxev+yf_5LzftPd+F7G_+W4PnTvibQ@mail.gmail.com>
From: Heikki Linnakangas <hlinnaka@iki.fi>
Message-ID: <cffe0dd6-8eba-4856-68cf-476fdb8a1235@iki.fi>
Date: Thu, 15 Oct 2020 17:59:13 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.3.1
MIME-Version: 1.0
In-Reply-To: 
 <CA+HiwqGYu4bkvFufvnW2Rxev+yf_5LzftPd+F7G_+W4PnTvibQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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