Re: [BUGS] BUG #14890: Error grouping by same column twice using FDW - Mailing list pgsql-bugs

On Thu, Nov 9, 2017 at 5:29 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
>
>
> In the attached patch, I have removed apply_pathtarget_labeling_to_tlist()
> call. Since we are anyways checking that whether the GROUP BY expression is
> entirely present in a target list or not, we can safely create a tle for
> that
> GROUP BY expression and append that to the new targetlist by making sure
> that
> its sortgroupref is also transferred, eliminating the need for relabeling
> afterwords.
>
> Now with these changes i.e. as we have moved setting sortgroupref for tle
> inside the if block itself, we don't have to reset the sortgroupref for any
> ORDER BY expression. That code is removed. This also eliminated the need for
> copying the pathtarget.

Some review comments.

+   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.
This isn't a real problem, since we push only immutable expressions, but
nonetheless something to avoid if we can.

That brings another question, do we really bother to pass duplicate entries in
the GROUP BY clause as they are? Can we just pass a single one? The result will
be the same since both these entries are immutable. May be we should pass
repeated entries in GROUP BY clause just in case, we somehow manage to ship
non-immutable expressions in future. Thoughts?

-     * have those arising from ORDER BY clause. These sortgrouprefs may be
-     * different from those in the plan's targetlist.

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.

+            /*
+             * Pushable, add to tlist. We need to create a tle for this
+             * expression and need to transfer the sortgroupref too. We cannot
+             * use add_to_flat_tlist() here as it avoids the duplicate entries
+             * in the targetlist but here we want those duplicate entries as
+             * there can be multiple GROUP BY expressions pointing to the same
+             * column at different positions.
+             */
+            tle = makeTargetEntry((Expr *) expr, list_length(tlist) + 1, NULL,
+                                  false);
+            tle->ressortgroupref = sgref;
+            tlist = lappend(tlist, tle);

May be we want to use add_flat_to_tlist() when sgref is not present to avoid
duplicate entries.



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


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs