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
