On Wed, Nov 15, 2017 at 2:36 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
>>
>> As I alluded to upthread, I suspect that dropping ORDER BY markings from
>> the tlist is likely to break some cases (that is, the planner may expect
>> the output of the foreign scan to include those columns). If this isn't
>> fully exercised by the existing tests then we definitely need more tests.
>
>
> We do have test-coverage where ORDER BY and GROUP BY have common entries.
>
> All ORDER BY expressions are part of the grouping targets and they must be
> either plain vars or aggregate function. Plain vars are anyways part of the
> GROUP BY expression otherwsie we will end-up in a error. And for aggregate
> function, we do include those in a targetlist.
All the ORDER BY expressions are part of the targetlist. If the whole
expression is pushable, we check if the Vars and Aggrefs involved in
that expressions are pushable and include those in the targetlist to
the pushed down. If they are not pushable, we do not push
aggregate/grouping. So, we are good wrt that. I also verified that the
current tests are enough.
I am fine with the patch. It introduces a lot of expected output diff,
since we always deparse GROUP BY clause in positional notation. That's
fine. There is no point in adding extra logic to use positional
notation only when there are duplicate entries in the GROUP BY clause.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
