From 48d9d8e1318eb1d4b94d7f02a86b9e9716369947 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sat, 7 Jul 2007 20:46:45 +0000
Subject: [PATCH] Fix a couple of planner bugs introduced by the new ability to
discard ORDER BY <constant> as redundant. One is that this means
query_planner() has to canonicalize pathkeys even when the query jointree is
empty; the canonicalization was always a no-op in such cases before, but no
more. Also, we have to guard against thinking that a set-returning function
is "constant" for this purpose. Add a couple of regression tests for these
evidently under-tested cases. Per report from Greg Stark and subsequent
experimentation.
---
src/backend/optimizer/path/equivclass.c | 16 +++++++------
src/backend/optimizer/plan/planmain.c | 14 ++++++++++-
src/test/regress/expected/select.out | 32 +++++++++++++++++++++++++
src/test/regress/sql/select.sql | 16 +++++++++++++
4 files changed, 70 insertions(+), 8 deletions(-)
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index b6503ef193b..fc862438ffb 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -10,7 +10,7 @@
* Portions Copyright (c) 1994, Regents of the University of California
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/optimizer/path/equivclass.c,v 1.2 2007/01/22 20:00:39 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/optimizer/path/equivclass.c,v 1.3 2007/07/07 20:46:45 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -328,8 +328,8 @@ add_eq_member(EquivalenceClass *ec, Expr *expr, Relids relids,
/*
* No Vars, assume it's a pseudoconstant. This is correct for
* entries generated from process_equivalence(), because a WHERE
- * clause can't contain aggregates and non-volatility was checked
- * before process_equivalence() ever got called. But
+ * clause can't contain aggregates or SRFs, and non-volatility was
+ * checked before process_equivalence() ever got called. But
* get_eclass_for_sort_expr() has to work harder. We put the tests
* there not here to save cycles in the equivalence case.
*/
@@ -428,13 +428,15 @@ get_eclass_for_sort_expr(PlannerInfo *root,
false, expr_datatype);
/*
- * add_eq_member doesn't check for volatile functions or aggregates,
- * but such could appear in sort expressions, so we have to check
- * whether its const-marking was correct.
+ * add_eq_member doesn't check for volatile functions, set-returning
+ * functions, or aggregates, but such could appear in sort expressions;
+ * so we have to check whether its const-marking was correct.
*/
if (newec->ec_has_const)
{
- if (newec->ec_has_volatile || contain_agg_clause((Node *) expr))
+ if (newec->ec_has_volatile ||
+ expression_returns_set((Node *) expr) ||
+ contain_agg_clause((Node *) expr))
{
newec->ec_has_const = false;
newem->em_is_const = false;
diff --git a/src/backend/optimizer/plan/planmain.c b/src/backend/optimizer/plan/planmain.c
index c31f7d5aa65..f8ff3a71508 100644
--- a/src/backend/optimizer/plan/planmain.c
+++ b/src/backend/optimizer/plan/planmain.c
@@ -14,7 +14,7 @@
*
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/optimizer/plan/planmain.c,v 1.101 2007/05/04 01:13:44 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/optimizer/plan/planmain.c,v 1.102 2007/07/07 20:46:45 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -106,9 +106,21 @@ query_planner(PlannerInfo *root, List *tlist,
*/
if (parse->jointree->fromlist == NIL)
{
+ /* We need a trivial path result */
*cheapest_path = (Path *)
create_result_path((List *) parse->jointree->quals);
*sorted_path = NULL;
+ /*
+ * We still are required to canonicalize any pathkeys, in case
+ * it's something like "SELECT 2+2 ORDER BY 1".
+ */
+ root->canon_pathkeys = NIL;
+ root->query_pathkeys = canonicalize_pathkeys(root,
+ root->query_pathkeys);
+ root->group_pathkeys = canonicalize_pathkeys(root,
+ root->group_pathkeys);
+ root->sort_pathkeys = canonicalize_pathkeys(root,
+ root->sort_pathkeys);
return;
}
diff --git a/src/test/regress/expected/select.out b/src/test/regress/expected/select.out
index d58c8d2811b..2936f0306ab 100644
--- a/src/test/regress/expected/select.out
+++ b/src/test/regress/expected/select.out
@@ -736,3 +736,35 @@ SELECT * FROM foo ORDER BY f1 DESC NULLS LAST;
(7 rows)
+--
+-- Test some corner cases that have been known to confuse the planner
+--
+-- ORDER BY on a constant doesn't really need any sorting
+SELECT 1 AS x ORDER BY x;
+ x
+---
+ 1
+(1 row)
+
+-- But ORDER BY on a set-valued expression does
+create function sillysrf(int) returns setof int as
+ 'values (1),(10),(2),($1)' language sql immutable;
+select sillysrf(42);
+ sillysrf
+----------
+ 1
+ 10
+ 2
+ 42
+(4 rows)
+
+select sillysrf(-1) order by 1;
+ sillysrf
+----------
+ -1
+ 1
+ 2
+ 10
+(4 rows)
+
+drop function sillysrf(int);
diff --git a/src/test/regress/sql/select.sql b/src/test/regress/sql/select.sql
index 8c92168c9b8..bb1fc9b9347 100644
--- a/src/test/regress/sql/select.sql
+++ b/src/test/regress/sql/select.sql
@@ -186,3 +186,19 @@ SELECT * FROM foo ORDER BY f1;
SELECT * FROM foo ORDER BY f1 NULLS FIRST;
SELECT * FROM foo ORDER BY f1 DESC;
SELECT * FROM foo ORDER BY f1 DESC NULLS LAST;
+
+--
+-- Test some corner cases that have been known to confuse the planner
+--
+
+-- ORDER BY on a constant doesn't really need any sorting
+SELECT 1 AS x ORDER BY x;
+
+-- But ORDER BY on a set-valued expression does
+create function sillysrf(int) returns setof int as
+ 'values (1),(10),(2),($1)' language sql immutable;
+
+select sillysrf(42);
+select sillysrf(-1) order by 1;
+
+drop function sillysrf(int);
--
GitLab