Re: [HACKERS] Runtime Partition Pruning

PFA the updated patches.

On Tue, Nov 14, 2017 at 11:45 AM, Beena Emerson <memissemerson@gmail.com> wrote:
> 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