Re: Aggregate Push Down - Performing aggregation on foreign server

On Thu, Oct 20, 2016 at 12:49 PM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)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