On Thu, Oct 20, 2016 at 12:49 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote:
> The patch compiles and make check-world doesn't show any failures. > > >> > > > > > > I have tried it. Attached separate patch for it. > > However I have noticed that istoplevel is always false (at-least for the > > testcase we have, I get it false always). And I also think that taking > > this decision only on PlanState is enough. Does that in attached patch. > > To fix this, I have passed deparse_namespace to the private argument and > > accessed it in get_special_variable(). Changes looks very simple. Please > > have a look and let me know your views. > > This issue is not introduced by the changes done for the aggregate push > > down, it only got exposed as we now ship expressions in the target list. > > Thus I think it will be good if we make these changes separately as new > > patch, if required. > > > > > The patch looks good and pretty simple. > > + * expression. However if underneath PlanState is ForeignScanState, > then > + * don't force parentheses. > We need to explain why it's safe not to add paranthesis. The reason > this function adds paranthesis so as to preserve any operator > precedences imposed by the expression tree of which this IndexVar is > part. For ForeignScanState, the Var may represent the root of the > expression tree, and thus not need paranthesis. But we need to verify > this and explain it in the comments. > > As you have explained this is an issue exposed by this patch; > something not must have in this patch. If committers feel that > aggregate pushdown needs this fix as well, please provide a patch > addressing the above comment. > > Sure. > > Ok. PFA patch with cosmetic changes (mostly rewording comments). Let > me know if those look good. I am marking this patch is ready for > committer. > Changes look good to me. Thanks for the detailed review. -- Jeevan B Chalke Principal Software Engineer, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
