Fix equivclass.c's not-quite-right strategy for handling X=X clauses.

The original coding correctly noted that these aren't just redundancies
(they're effectively X IS NOT NULL, assuming = is strict).  However, they
got treated that way if X happened to be in a single-member EquivalenceClass
already, which could happen if there was an ORDER BY X clause, for instance.
The simplest and most reliable solution seems to be to not try to process
such clauses through the EquivalenceClass machinery; just throw them back
for traditional processing.  The amount of work that'd be needed to be
smarter than that seems out of proportion to the benefit.

Per bug #5084 from Bernt Marius Johnsen, and analysis by Andrew Gierth.

diff --git a/src/backend/optimizer/README b/src/backend/optimizer/README
index 26e6dabae1ac73e66c91a76f0b844ef503fa8b80..43013727ed24efbf5e1b9e8db8ca84a32063e999 100644 (file)
--- a/src/backend/optimizer/README
+++ b/src/backend/optimizer/README
@@ -481,7 +481,10 @@ operator is mergejoinable, so there is no way for a volatile expression to
 get into EquivalenceClasses otherwise.  Aggregates are disallowed in WHERE
 altogether, so will never be found in a mergejoinable clause.)  This is just
 a convenience to maintain a uniform PathKey representation: such an
-EquivalenceClass will never be merged with any other.
+EquivalenceClass will never be merged with any other.  Note in particular
+that a single-item EquivalenceClass {a.x} is *not* meant to imply an
+assertion that a.x = a.x; the practical effect of this is that a.x could
+be NULL.
 
 An EquivalenceClass also contains a list of btree opfamily OIDs, which
 determines what the equalities it represents actually "mean".  All the

diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index ef797f67bec6e8e1f6152b5e3078af7b5c08a08a..f0a458b5fd41a4464dd2b4ba10462ff23aee3f82 100644 (file)
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -114,6 +114,19 @@ process_equivalence(PlannerInfo *root, RestrictInfo *restrictinfo,
        item1_relids = restrictinfo->left_relids;
        item2_relids = restrictinfo->right_relids;
 
+       /*
+        * Reject clauses of the form X=X.  These are not as redundant as they
+        * might seem at first glance: assuming the operator is strict, this is
+        * really an expensive way to write X IS NOT NULL.  So we must not risk
+        * just losing the clause, which would be possible if there is already
+        * a single-element EquivalenceClass containing X.  The case is not
+        * common enough to be worth contorting the EC machinery for, so just
+        * reject the clause and let it be processed as a normal restriction
+        * clause.
+        */
+       if (equal(item1, item2))
+               return false;                   /* X=X is not a useful equivalence */
+
        /*
         * If below outer join, check for strictness, else reject.
         */
@@ -152,13 +165,10 @@ process_equivalence(PlannerInfo *root, RestrictInfo *restrictinfo,
         *
         * 4. We find neither.  Make a new, two-entry EC.
         *
-        * Note: since all ECs are built through this process, it's impossible
-        * that we'd match an item in more than one existing EC.  It is possible
-        * to match more than once within an EC, if someone fed us something silly
-        * like "WHERE X=X".  (However, we can't simply discard such clauses,
-        * since they should fail when X is null; so we will build a 2-member EC
-        * to ensure the correct restriction clause gets generated.  Hence there
-        * is no shortcut here for item1 and item2 equal.)
+        * Note: since all ECs are built through this process or the similar
+        * search in get_eclass_for_sort_expr(), it's impossible that we'd match
+        * an item in more than one existing nonvolatile EC.  So it's okay to stop
+        * at the first match.
         */
        ec1 = ec2 = NULL;
        em1 = em2 = NULL;

diff --git a/src/test/regress/expected/select.out b/src/test/regress/expected/select.out
index 47c3e67b230f0ba8abbdbefec61539ea4140212a..449341739c91bc0085f02cd4c4962fbc2fc009a2 100644 (file)
--- a/src/test/regress/expected/select.out
+++ b/src/test/regress/expected/select.out
@@ -768,3 +768,19 @@ select sillysrf(-1) order by 1;
 (4 rows)
 
 drop function sillysrf(int);
+-- X = X isn't a no-op, it's effectively X IS NOT NULL assuming = is strict
+-- (see bug #5084)
+select * from (values (2),(null),(1)) v(k) where k = k order by k;
+ k 
+---
+ 1
+ 2
+(2 rows)
+
+select * from (values (2),(null),(1)) v(k) where k = k;
+ k 
+---
+ 2
+ 1
+(2 rows)
+

diff --git a/src/test/regress/sql/select.sql b/src/test/regress/sql/select.sql
index a9ddd5e5861eee963a3dde46d19a4343aec25b75..451fcf78d9ee82b738bed82353da57b65988d24c 100644 (file)
--- a/src/test/regress/sql/select.sql
+++ b/src/test/regress/sql/select.sql
@@ -202,3 +202,8 @@ select sillysrf(42);
 select sillysrf(-1) order by 1;
 
 drop function sillysrf(int);
+
+-- X = X isn't a no-op, it's effectively X IS NOT NULL assuming = is strict
+-- (see bug #5084)
+select * from (values (2),(null),(1)) v(k) where k = k order by k;
+select * from (values (2),(null),(1)) v(k) where k = k;