Re: [HACKERS] Runtime Partition Pruning - Mailing list pgsql-hackers

Hello Amul,

Thank you for reviewing.

On Fri, Nov 10, 2017 at 4:33 PM, amul sul <sulamul@gmail.com> wrote:
> On Thu, Nov 9, 2017 at 4:48 PM, Beena Emerson <memissemerson@gmail.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