On 7 April 2018 at 10:45, David Rowley <david.rowley@2ndquadrant.com> wrote:
> I'm looking over the rebased patches now.
I've made a complete read of 0001 and 0002 so far.
Your rebase looks fine.
After the complete read, I only have the following comments:
0001:
1. missing "the" before "partition key":
* Extract Params matching partition key and record if we got any.
2. Is this the property name we're going to stick with:
ExplainPropertyInteger("Subplans Pruned", NULL, nplans - nsubnodes, es);
Other ideas are: "Subplans Removed"
3. In the following comment I've used the word "hierarchy", but maybe
we need to add the word "flattened" before it.
* PartitionPruning - Encapsulates a hierarchy of PartitionRelPruning
4. Comment mentions "after init plan", but really we can only know the
value of an exec param during actual execution. So:
* Parameters that are safe to be used for partition pruning. execparams
* are not safe to use until after init plan.
maybe better as:
* Parameters that are safe to be used for partition pruning. execparams
* are not safe to use until the executor is running.
0002:
Looks fine. But if I was committing this, to give me confidence, I'd
want to know how the left_most_one table was generated.
I used:
#include <stdio.h>
int main(void)
{
int i = 1;
printf("0, ");
while (i < 256)
{
printf("%d, ", 31 - __builtin_clz(i));
if ((i & 0xf) == 0xf)
putchar('\n');
i++;
}
return 0;
}
Continuing to read 0003 and 0004 now.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
