Hi David,
First of all: Solid patch set with good documentation.
On 04/05/2018 09:41 AM, David Rowley wrote:
Seems mostly fair. I'm not a fan of using the term "unpruned" though.
I'll have a think. The "all" is meant in terms of what exists as
subnodes.
'included_parts' / 'excluded_parts' probably isn't better...
subplan_indexes and parent_indexes seem like better names, I agree.
More clear.
* 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.
0 is a valid subplan index. It is possible to make this happen, but
I'd need to subtract 1 everywhere I used the map. That does not seem
very nice. Seems more likely to result in bugs where we might forget
to do the - 1.
Did you want this because you'd rather have the palloc0() than the for
loop setting the array elements to -1? Or is there another reason?
I think that doing palloc0 would be confusing; -1 is more clear,
especially since it is used with 'allpartindexes' which is a Bitmapset.
Doing the variable renames as Amit suggests would be good.
I ran some tests (v50_v20) (make check-world passes), w/ and w/o
choose_custom_plan() being false, and seeing good performance results
without running into issues.
Maybe 0005 should be expanded in partition_prune.sql with the supported
cases to make those more clear.
Thanks !
Best regards,
Jesper
