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

On Sat, Nov 11, 2017 at 12:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> 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.
>

Understood.
Along with the last patch changes which does put duplicate entries if they
were referred in GROUP BY clause, in attached patch I have modified
deparsing
logic to emit GROUP BY column-number.

> 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.
>

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.

> regards, tom lane
>

Let me know if I missed any or wrongly interpreted anything over here.

Thanks
--
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company