Re: Aggregate Push Down - Performing aggregation on foreign server

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.

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.