Re: [HACKERS] Runtime Partition Pruning

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