Re: Internal error XX000 with enable_partition_pruning=on, pg 11 beta1 on Debian

David Rowley <david.rowley@2ndquadrant.com> writes:
> Anyway... Patch attached.  This is method 3 of the 3 methods I thought
> to fix this, so if this is not suitable then I'm out of ideas.

I started to look at this patch.  I think this is basically the right
direction to go in, but I'm not terribly happy with the details of the
data structure design.

First off, given that we're now going to have a Plan data structure
that accurately reflects the partition hierarchy, I wonder whether we
couldn't get rid of the fiddling around with lists of ints and lists of
lists of ints; aren't those basically duplicative?  They'd certainly be
so if we add the rel's RT index to PartitionedRelPruneInfo, but maybe
they are even without that.  Alvaro complained about the code associated
with those lists not being very idiomatic, but I'd like to just get rid
of the lists entirely.  I especially don't care for keeping the implicit
ordering assumptions in those lists (parents before children) when the
same info is now going to be explicit in a nearby structure.  (This
ties into the remarks I just made to Amit about getting rid of
executor-startup lock-taking logic.  Most of that change only makes
sense to do in v12, but I'd prefer that this patch not double down on
reliance on data structures we'd like to get rid of.)

Second, I still feel that you've got the sense of the prunable-subplans
bitmaps backwards, and I do not buy the micro-optimization argument you
made for doing it like that.  It complicates the code, yet the cost of
one bit in a bitmap is completely negligible compared to all the other
costs involved in having a per-partition subplan, whether or not that
subplan actually gets used in a particular run.

One very minor quibble is that I'd be inclined to represent the
PartitionPruningData struct like this:

typedef struct PartitionPruningData
{
    int            num_partrelprunedata;
    PartitionedRelPruningData partrelprunedata[FLEXIBLE_ARRAY_MEMBER];
} PartitionPruningData;

so we can allocate it with one palloc not two.  Also, in the Plan
representation, I'd suggest avoiding situations where a data structure
is sometimes a List of Lists of PartitionedRelPruneInfo and sometimes
just a single-level List.  Saving one ListCell isn't worth the code
complexity and error-proneness of that.

            regards, tom lane