PFA the updated patches. On Tue, Nov 14, 2017 at 11:45 AM, Beena Emerson <memissemer...@gmail.com> wrote: > Hello Amul, > > Thank you for reviewing. > > On Fri, Nov 10, 2017 at 4:33 PM, amul sul <sula...@gmail.com> wrote: >> On Thu, Nov 9, 2017 at 4:48 PM, Beena Emerson <memissemer...@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
0002-Implement-runtime-partiton-pruning_v2.patch
Description: Binary data
0001-Refactor-functions-and-structs-required-for-runtime_v2.patch
Description: Binary data