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