Re: BUG #14890: Error grouping by same column twice using FDW

Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> writes:
> On Thu, Nov 9, 2017 at 5:29 PM, Jeevan Chalke
> <jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>> + Remote SQL: SELECT c2, c2 FROM "S 1"."T 1" WHERE ((c2 = 6)) GROUP BY c2, c2

> GROUP BY 1, 2 is changed to GROUP BY c2, c2 which is technically wrong. The
> remote planner will think that both the GROUP BY entries refer to one of the
> (possibly the first) entry in the SELECT clause. That's not what really it is.

Yeah. I'm inclined to think that part of what needs to happen here is for
postgres_fdw to change over to always emitting GROUP BY column-number,
so that the grouping columns are clearly matched up with the tlist entries
it's considering, and the remote parser is certain to build
ressortgrouprefs that match what we thought was happening locally.

As you say, we can probably get away without that as long as we don't push
mutable grouping expressions ... but just because we think a grouping
expression is immutable at our end doesn't necessarily mean that it is at
the far end. Also, in view of the (as yet unfixed) bug discussed in
https://www.postgresql.org/message-id/flat/7dbdcf5c-b5a6-ef89-4958-da212fe10176(at)iki(dot)fi
there's no hope of extending postgres_fdw to push GROUPING SETS correctly
unless it is able to distinguish textually-equal grouping columns.

> May be we were not explaining this correctly earlier. The sortgrouprefs of
> GROUP BY clause can not be different between those two tlists. The difference
> is really the absence of ORDER BY entries. May be we should add some tests
> where there some entries common between ORDER BY and GROUP BY.

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.

regards, tom lane