Re: [HACKERS] Runtime Partition Pruning

On 2018/04/05 12:14, Amit Langote wrote:
> I will post comments on your v19 later today.

I looked at it and here are my thoughts on it after having for the first
time looked very closely at it.

* Regarding PartitionPruneInfo:

I think the names of the fields could be improved -- like pruning_steps
instead prunesteps, unpruned_parts instead of allpartindexs. The latter
is even a bit misleading because it doesn't in fact contain *all*
partition indexes, only those that are unpruned, because each either has a
subpath or it appears in (unpruned) partitioned_rels list. Also, I didn't
like the name subnodeindex and subpartindex very much. subplan_indexes
and parent_indexes would sound more informative to me.

* make_partition_pruneinfo has a parameter resultRelations that's not used
anywhere

* In make_partition_pruneinfo()

allsubnodeindex = palloc(sizeof(int) * root->simple_rel_array_size);
allsubpartindex = palloc(sizeof(int) * root->simple_rel_array_size);

I think these arrays need to have root->simple_rel_array_size + 1
elements, because they're subscripted using RT indexes which start at 1.

* Also in make_partition_pruneinfo()

/* Initialize to -1 to indicate the rel was not found */
for (i = 0; i < root->simple_rel_array_size; i++)
{
allsubnodeindex[i] = -1;
allsubpartindex[i] = -1;
}

Maybe, allocate the arrays above mentioned using palloc0 and don't do this
initialization. Instead make the indexes that are stored in these start
with 1 and consider 0 as invalid entries.

* Regarding the code added in execPartition.c and execPartition.h:

I wondered why PartitionedRelPruning is named the way it is. I saw many
parallels with PartitionDispatchData both in terms of the main thing it
consists of, that is, the map that translates partition indexes as in
partition descriptor to that of subplans or of some other executor
structure. Also, I imagine you tried to mimic PartitionTupleRouting with
PartitionPruning but not throughout. For example, tuple routing struct
pointer variables are throughout called proute, whereas PartitionPruning
ones are called partprune instead of, say, pprune. Consistency would
help, imho.

* Instead of nesting PartitionedRelPruning inside another, just store them
in a global flat array in the PartitionPruning, like PartitionTupleRouting
does for PartitionDispatch of individual partitioned tables in the tree.

typedef struct PartitionedRelPruning
{
int nparts;
int *subnodeindex;
struct PartitionedRelPruning **subpartprune;

* I don't see why there needs to be nparts in the above, because it
already has a PartitionPruneContext member which has that information.

In fact, I made most of changes myself while going through the code.
Please see attached the delta patch. It also tweaks quite a few other
things including various comments. I think parts of it apply to 0001,
0003, and 0004 patches. See if this looks good to you.

Thanks,
Amit