From 48d9d8e1318eb1d4b94d7f02a86b9e9716369947 Mon Sep 17 00:00:00 2001 From: Tom Lane 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 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 b6503ef193..fc862438ff 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 c31f7d5aa6..f8ff3a7150 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 d58c8d2811..2936f0306a 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 8c92168c9b..bb1fc9b934 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); -- 2.40.0