Date: Fri, 16 Oct 2020 11:45:41 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>,
	Daniel Gustafsson <daniel@yesql.se>,
	Etsuro Fujita <etsuro.fujita@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: partition routing layering in nodeModifyTable.c
Message-ID: <20201016144541.GA26250@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+HiwqHtCWLdK-LO=NEsvOdHx+7yv4mE_zYK0i3BH7dXb-wxog@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2020-Oct-16, Amit Langote wrote:

> On Thu, Oct 15, 2020 at 11:59 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > On Wed, Oct 14, 2020 at 6:04 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> > And if we removed
> > ri_PartitionInfo->pi_PartitionToRootMap, and always used
> > ri_ChildToRootMap for it.
> 
> Done in the attached.

Hmm...  Overall I like the simplification.

> > Maybe remove PartitionRoutingInfo struct altogether, and just move its
> > fields directly to ResultRelInfo.
> 
> If we do that, we'll end up with 3 notations for the same thing across
> releases: In v10 and v11, PartitionRoutingInfos members are saved in
> arrays in ModifyTableState, totally detached from the partition
> ResultRelInfos.  In v12 (3f2393edef), we moved them into ResultRelInfo
> but chose to add them into a sub-struct (PartitionRoutingInfo), which
> in retrospect was not a great decision.  Now if we pull them into
> ResultRelInfo, we'll have invented the 3rd notation.  Maybe that makes
> things hard when back-patching bug-fixes?

I don't necessarily agree that PartitionRoutingInfo was such a bad idea.
In fact I wonder if we shouldn't move *more* stuff into it
(ri_PartitionCheckExpr), and keep struct ResultRelInfo clean of
partitioning-related stuff (other than ri_PartitionInfo and
ri_PartitionRoot); there are plenty of ResultRelInfos that are not
partitions, so I think it makes sense to keep the split.  I'm thinking
that the ChildToRootMap should continue to be in PartitionRoutingInfo.

Maybe what we need in order to keep the initialization "lazy enough" is
some inline functions that act as getters, initializing members of
PartitionRoutingInfo when first needed.  (This would probably need
boolean flags, to distinguish "hasn't been set up yet" from "it is not
needed for this partition" for each member that requires it).

BTW it is curious that ExecInitRoutingInfo is called both in
ExecInitPartitionInfo() (from ExecFindPartition when the ResultRelInfo
for the partition is not found) *and* from ExecFindPartition again, when
the ResultRelInfo for the partition *is* found.  Doesn't this mean that
ri_PartitionInfo is set up twice for the same partition?