On Fri, 8 Mar 2024 at 00:54, Ashutosh Bapat
<ashutosh.bapat....@gmail.com> wrote:
>
> On Thu, Mar 7, 2024 at 4:39 PM David Rowley <dgrowle...@gmail.com> wrote:
>> I think the fix should go in appendOrderByClause(). It's at that
>> point we look for the EquivalenceMember for the relation and can
>> easily discover if the em_expr is a Const. I think we can safely just
>> skip doing any ORDER BY <const> stuff and not worry about if the
>> literal format of the const will appear as a reference to an ordinal
>> column position in the ORDER BY clause.
>
> deparseSortGroupClause() calls deparseConst() with showtype = 1.
> appendOrderByClause() may want to do something similar for consistency. Or
> remove it from deparseSortGroupClause() as well?
The fix could also be to use deparseConst() in appendOrderByClause()
and have that handle Const EquivalenceMember instead. I'd rather just
skip them. To me, that seems less risky than ensuring deparseConst()
handles all Const types correctly.
Also, as far as adjusting GROUP BY to eliminate Consts, I don't think
that's material for a bug fix. If we want to consider doing that,
that's for master only.
>> I wonder if we need a test...
>
> Yes.
I've added two of those in the attached.
I also changed the way the delimiter stuff works as the exiting code
seems to want to avoid having a bool flag to record if we're adding
the first item. The change I'm making means the bool flag is now
required, so we may as well use that flag to deal with the delimiter
append too.
David
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 8fc66fa11c..a30f00179e 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -3912,13 +3912,12 @@ appendOrderByClause(List *pathkeys, bool has_final_sort,
{
ListCell *lcell;
int nestlevel;
- const char *delim = " ";
StringInfo buf = context->buf;
+ bool gotone = false;
/* Make sure any constants in the exprs are printed portably */
nestlevel = set_transmission_modes();
- appendStringInfoString(buf, " ORDER BY");
foreach(lcell, pathkeys)
{
PathKey *pathkey = lfirst(lcell);
@@ -3951,6 +3950,25 @@ appendOrderByClause(List *pathkeys, bool has_final_sort,
em_expr = em->em_expr;
+ /*
+ * If the member is a Const expression then we needn't add it
to the
+ * ORDER BY clause. This can happen in UNION ALL queries where
the
+ * union child targetlist has a Const. Adding these would be
wasteful,
+ * but also, for INT columns, an integer literal will be seen
as an
+ * ordinal column position rather than a value to sort by, so
we must
+ * skip these.
+ */
+ if (IsA(em_expr, Const))
+ continue;
+
+ if (!gotone)
+ {
+ appendStringInfoString(buf, " ORDER BY ");
+ gotone = true;
+ }
+ else
+ appendStringInfoString(buf, ", ");
+
/*
* Lookup the operator corresponding to the strategy in the
opclass.
* The datatype used by the opfamily is not necessarily the
same as
@@ -3965,7 +3983,6 @@ appendOrderByClause(List *pathkeys, bool has_final_sort,
pathkey->pk_strategy, em->em_datatype,
em->em_datatype,
pathkey->pk_opfamily);
- appendStringInfoString(buf, delim);
deparseExpr(em_expr, context);
/*
@@ -3975,7 +3992,6 @@ appendOrderByClause(List *pathkeys, bool has_final_sort,
appendOrderBySuffix(oprid, exprType((Node *) em_expr),
pathkey->pk_nulls_first, context);
- delim = ", ";
}
reset_transmission_modes(nestlevel);
}
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out
b/contrib/postgres_fdw/expected/postgres_fdw.out
index c355e8f3f7..f3339b4ac8 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -919,6 +919,44 @@ EXPLAIN (VERBOSE, COSTS OFF)
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
(6 rows)
+-- Ensure we don't push ORDER BY expressions which are Consts at the UNION
+-- child level to the foreign server.
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM (
+ SELECT 1 AS type,c1 FROM ft1
+ UNION ALL
+ SELECT 2 AS type,c1 FROM ft2
+) ORDER BY type,c1;
+ QUERY PLAN
+---------------------------------------------------------------------------------
+ Merge Append
+ Sort Key: (1), ft1.c1
+ -> Foreign Scan on public.ft1
+ Output: 1, ft1.c1
+ Remote SQL: SELECT "C 1" FROM "S 1"."T 1" ORDER BY "C 1" ASC NULLS
LAST
+ -> Foreign Scan on public.ft2
+ Output: 2, ft2.c1
+ Remote SQL: SELECT "C 1" FROM "S 1"."T 1" ORDER BY "C 1" ASC NULLS
LAST
+(8 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM (
+ SELECT 1 AS type,c1 FROM ft1
+ UNION ALL
+ SELECT 2 AS type,c1 FROM ft2
+) ORDER BY type;
+ QUERY PLAN
+---------------------------------------------------
+ Merge Append
+ Sort Key: (1)
+ -> Foreign Scan on public.ft1
+ Output: 1, ft1.c1
+ Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
+ -> Foreign Scan on public.ft2
+ Output: 2, ft2.c1
+ Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
+(8 rows)
+
-- user-defined operator/function
CREATE FUNCTION postgres_fdw_abs(int) RETURNS int AS $$
BEGIN
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql
b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 812e7646e1..bc8fdb6419 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -362,6 +362,22 @@ EXPLAIN (VERBOSE, COSTS OFF)
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft2 ORDER BY ft2.c1, ft2.c3 collate "C";
+-- Ensure we don't push ORDER BY expressions which are Consts at the UNION
+-- child level to the foreign server.
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM (
+ SELECT 1 AS type,c1 FROM ft1
+ UNION ALL
+ SELECT 2 AS type,c1 FROM ft2
+) ORDER BY type,c1;
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM (
+ SELECT 1 AS type,c1 FROM ft1
+ UNION ALL
+ SELECT 2 AS type,c1 FROM ft2
+) ORDER BY type;
+
-- user-defined operator/function
CREATE FUNCTION postgres_fdw_abs(int) RETURNS int AS $$
BEGIN
