Re: Internal error XX000 with enable_partition_pruning=on, pg 11beta1 on Debian - Mailing list pgsql-hackers

On 2018/07/19 22:03, David Rowley wrote:
> v3-0001-Fix-run-time-partition-pruning-for-UNION-ALL-pare.patch

Thanks for updating the patch.

I studied this patch today and concluded that it's going a bit too far by
carrying the nested partition pruning info structures from the planner all
the way into the executor.

I understood the root cause of this issue as that make_partition_pruneinfo
trips when UNION ALL's parent subquery, instead of the actual individual
partitioned root tables, is treated as the root parent rel when converting
prunequals using appenrel_adjust_*.  That happens because of a flattened
partitioned_rels list whose members are all assumed by
make_partition_pruneinfo to have the same root parent and that it is an
actual partitioned table.  That assumption fails in this case because the
parent is really the UNION ALL subquery rel.

I think the fix implemented in the patch by modifying allpaths.c is
correct, whereby the partition hierarchies are preserved by having nested
Lists of partitioned rels.  So, the partitioned_rels List corresponding to
UNION ALL subquery parent itself contains Lists corresponding to
partitioned tables appearing under it.  With that,
make_partition_pruneinfo (actually, make_partitionedrel_pruneinfo in the
patch) can correctly perform its work for every sub-List, because for each
sub-List, it can identify the correct root partitioned table parent to use.

But I don't think the result of make_partition_pruneinfo itself has to be
List of PartitionedRelPruneInfo nested under PartitionPruneInfo.  I gather
that each PartitionPruneInfo corresponds to each root partitioned table
and a PartitionedRelPruneInfo contains the actual pruning information,
which is created for every partitioned table (non-leaf tables), including
the root tables.  I don't think such nesting is necessary.  I think that
just like flattened partitioned_rels list, we should put flattened list of
PartitionedRelPruneInfo into the Append or MergeAppend plan.  No need for
nesting PartitionedRelPruneInfo under PartitionPruneInfo.

We create a relid_subplan_map from the flattened list of sub-plans, where
sub-plans of leaf partitions of different partitioned tables appear in the
same list.  Similarly, I think, we should create relid_subpart_map from
the flattened list of partitioned_rels, where partitioned rel RT indexes
coming from different partitioned tables appear in the same list.
Currently relid_subpart_map seems to be constructed separately for each
sub-List of nested partitioned_rels list, so while subplan_map of each
PartitionedRelPruneInfo contains indexes into a global array of leaf
partition sub-plans, subpart_map contains indexes into local array of
PartitionedRelPruneInfo for that partitioned table.  But, I think there is
not a big hurdle in making even the latter contain indexes into global
array of PartitionedRelPruneInfos of *all* partitioned tables.

On the executor side, instead of having PartitionedRelPruningData be
nested under PartitionPruningData, which in turn are stored in the
top-level PartitionPruneState, store them directly in PartitionPruneState,
since we're making planner put global indexes into subpart_map.  Slight
adjustment seems to be needed to make ExecInitFindMatchingSubPlans and
ExecFindMatchingSubPlans skip the PartitionedRelPruningData of non-root
tables, because find_matching_subplans_recurse takes care of recursing to
non-root ones.  Actually, not skipping them seems to cause wrong result.

To verify that such an approach would actually work, I modified the
relevant parts of your patch and confirmed that it does.  See attached a
delta patch.

Thanks,
Amit

PS: Other than the main patch, I think it would be nice to add a RT index
field to PartitionedRelPruneInfo in addition to the existing Oid field.
It would help to identify and fetch the Relation from a hypothetical
executor-local array of Relation pointers which is addressable by RT indexes.