Re: [HACKERS] Runtime Partition Pruning

Hello Amul,

Thank you for reviewing.

On Fri, Nov 10, 2017 at 4:33 PM, amul sul <sulamul(at)gmail(dot)com> wrote:
> On Thu, Nov 9, 2017 at 4:48 PM, Beena Emerson <memissemerson(at)gmail(dot)com> wrote:
>> Hello all,
>>
>> Here is the updated patch which is rebased over v10 of Amit Langote's
>> path towards faster pruning patch [1]. It modifies the PartScanKeyInfo
>> struct to hold expressions which is then evaluated by the executor to
>> fetch the correct partitions using the function.
>>
>
> Hi Beena,
>
> I have started looking into your patch, here few initial comments
> for your 0001 patch:
>
> 1.
> 351 + * Evaluate and store the ooutput of ExecInitExpr for each
> of the keys.
>
> Typo: ooutput

Corrected.

>
> 2.
> 822 + if (IsA(constexpr, Const) &&is_runtime)
> 823 + continue;
> 824 +
> 825 + if (IsA(constexpr, Param) &&!is_runtime)
> 826 + continue;
> 827 +
>
> Add space after '&&'

Done.

>
> 3.
> 1095 + * Generally for appendrel we don't fetch the clause from the the
>
> Typo: Double 'the'
>
> 4.
> 272 -/*-------------------------------------------------------------------------
> 273 + /*-------------------------------------------------------------------------
>
> Unnecessary hunk.

Removed.

>
> 5.
> 313 + Node *n =
> eval_const_expressions_from_list(estate->es_param_list_info, val);
> 314 +
>
> Crossing 80 column window. Same at line # 323 & 325

Fixed.

>
> 6.
> 315 + keys->eqkeys_datums[i++] = ((Const *) n)->constvalue;
>
> Don’t we need a check for IsA(n, Const) or assert ?

added

>
> 7.
> 1011 + if (prmList)
> 1012 + context.boundParams = prmList; /* bound Params */
> 1013 + else
> 1014 + context.boundParams = NULL;
>
> No need of prmList null check, context.boundParams = prmList; is enough.
>
> 8. It would be nice if you create a separate patch where you are moving
> PartScanKeyInfo and exporting function declaration.

This is in 0001.

>
> 9. Could you please add few regression tests, that would help in
> review & testing.

I will make a seperate regression patch and submit soon.

>
> 10. Could you please rebase your patch against latest "path toward faster
> partition pruning" patch by Amit.

The following is rebased over v11 Amit's patch [1]

[1] https://www.postgresql.org/message-id/62d21a7b-fea9-f2d7-c33a-8caa12eca612%40lab.ntt.co.jp

--

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company