Re: [HACKERS] Partition-wise join for join between (declaratively)partitioned tables

On 2017/09/04 21:32, Ashutosh Bapat wrote:
> On Mon, Sep 4, 2017 at 10:04 AM, Amit Langote wrote:
>> By the way, if you want to get rid of PartitionedChildRelInfo, you can do
>> that as long as you find some other way of putting together the
>> partitioned_rels list to add into the ModifyTable (Append/MergeAppend)
>> node created for the root partitioned table.  Currently,
>> PartitionedChildRelInfo (and the root->pcinfo_list) is the way for
>> expand_inherited_rtentry() to pass that information to the planner's
>> path-generating code.  We may be able to generate that list when actually
>> creating the path using set_append_rel_pathlist() or
>> inheritance_planner(), without having created a PartitionedChildRelInfo
>> node beforehand.
> 
> AFAIU, the list contained RTIs of the relations, which didnt' have
> corresponding AppendRelInfos to lock those relations. Now that we
> create AppendRelInfos even for partitioned partitions, I don't think
> we need the list to take care of the locks. Is there any other reason
> why we maintain that list (apart from the trigger case I have raised
> and Fujita-san says that the list is not required in that case as
> well.)

We do *need* the list in ModifyTable (Append/MergeAppend) node itself.  We
can, however, get rid of the PartitionedChildRelInfo node that carries the
partitioned child RT indexes from an earlier planning phase
(expand_inherited_rtentry) to a later phase
(create_{modifytable|append|merge_append}_path).  The later phase can
build that list from the AppendRelInfos that you mention we now [1] build.

>>> Though I haven't read the patch yet, I think the above code is useless.
>>> And I proposed a patch to clean it up before [1].  I'll add that patch to
>>> the next commitfest.
>>
>> +1.
> +1. Will Fujita-san's patch also handle getting rid of partitioned_rels list?

As Fujita-san mentioned, his patch won't.  Actually, I suppose he didn't
say that partitioned_rels itself is useless, just that its particular
usage in ExecInitModifyTable is.  We still need that list for planner to
tell the executor that there are some RT entries the latter would need to
lock before executing a given plan.  Without that dedicated list, the
executor cannot know at all that certain tables in the partition tree
(viz. the partitioned ones) need to be locked.  I mentioned the reason -
(Merge)Append.subplans, ModifyTable.resultRelations does not contain
respective entries corresponding to the partitioned tables, and
traditionally, the executor looks at those lists to figure out the tables
to lock.

Thanks,
Amit

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=30833ba154