Re: Aggregate Push Down - Performing aggregation on foreign server - Mailing list pgsql-hackers

The patch compiles and make check-world doesn't show any failures.

>>
>> >
>> > ExecInitForeignScan() while determining scan tuple type from passed
>> > fdw_scan_tlist, has target list containing Vars with varno set to
>> > INDEX_VAR. Due to which while deparsing we go through
>> > resolve_special_varno() and get_special_variable() function which
>> > forces parenthesis around the expression.
>> > I can't think of any easy and quick solution here. So keeping this
>> > as is. Input will be welcome or this can be done separately later.
>> >
>>
>> I guess, you can decide whether or not to add paranthesis by looking
>> at the corresponding namespace in the deparse_context. If the
>> planstate there is ForeignScanState, you may skip the paranthesis if
>> istoplevel is true. Can you please check if this works?
>
>
> 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.

>> >
>> >
>> > Moved testcases after Join testcases.
>> > However I have not made any capitalization changes. I see many tests
>> > like those elsewhere in the file too. Let me know if that's the policy
>> > to have appropriate capitalization in test-case. I don't want violate
>> > the policy if any and will prefer to do the changes as per the policy.
>>
>> This file has used different styles for different sets. But we usually
>> adopt the style from surrounding testcases. The testcases surrounding
>> aggregate testcase block are capitalized. It will look odd to have
>> just the aggregate tests using different style.
>
>
> I see mixed use of capitalization in the file and use of it is adhoc too.
> So not convinced with your argument yet. I don't think there is any policy
> for that; otherwise use of capitalization in this file would have been
> consistent already. Can we defer this to the committer's opinion.
>

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.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company