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