Re: BUG #17618: unnecessary filter column <> text even after adding index

Richard Guo <guofenglinux@gmail.com> writes:
> I've updated the patch according to the suggestions as in v4.  Thanks
> for reviewing this patch!

I was about ready to commit this when I re-read your initial comment
and realized that there's a second way to fix it.  We can improve
predtest.c so that it understands that "x = true" implies "x" and
so on, whereupon the existing logic in create_bitmap_scan_plan
handles the case correctly.  This is pretty nearly the same code as
in your v4, except that it's in a considerably less hot code path, plus
there's at least some chance that it could be useful for other purposes.
So I think I like this way better.  Thoughts?

            regards, tom lane

diff --git a/contrib/btree_gin/expected/bool.out b/contrib/btree_gin/expected/bool.out
index 207a3f2328..b379622baf 100644
--- a/contrib/btree_gin/expected/bool.out
+++ b/contrib/btree_gin/expected/bool.out
@@ -92,7 +92,7 @@ EXPLAIN (COSTS OFF) SELECT * FROM test_bool WHERE i=true ORDER BY i;
  Sort
    Sort Key: i
    ->  Bitmap Heap Scan on test_bool
-         Filter: i
+         Recheck Cond: i
          ->  Bitmap Index Scan on idx_bool
                Index Cond: (i = true)
 (6 rows)
@@ -119,3 +119,15 @@ EXPLAIN (COSTS OFF) SELECT * FROM test_bool WHERE i>true ORDER BY i;
                Index Cond: (i > true)
 (6 rows)

+-- probably sufficient to check just this one:
+EXPLAIN (COSTS OFF) SELECT * FROM test_bool WHERE i=false ORDER BY i;
+                QUERY PLAN
+-------------------------------------------
+ Sort
+   Sort Key: i
+   ->  Bitmap Heap Scan on test_bool
+         Recheck Cond: (NOT i)
+         ->  Bitmap Index Scan on idx_bool
+               Index Cond: (i = false)
+(6 rows)
+
diff --git a/contrib/btree_gin/sql/bool.sql b/contrib/btree_gin/sql/bool.sql
index dad2ff32b8..08f2986e8c 100644
--- a/contrib/btree_gin/sql/bool.sql
+++ b/contrib/btree_gin/sql/bool.sql
@@ -25,3 +25,5 @@ EXPLAIN (COSTS OFF) SELECT * FROM test_bool WHERE i<=true ORDER BY i;
 EXPLAIN (COSTS OFF) SELECT * FROM test_bool WHERE i=true ORDER BY i;
 EXPLAIN (COSTS OFF) SELECT * FROM test_bool WHERE i>=true ORDER BY i;
 EXPLAIN (COSTS OFF) SELECT * FROM test_bool WHERE i>true ORDER BY i;
+-- probably sufficient to check just this one:
+EXPLAIN (COSTS OFF) SELECT * FROM test_bool WHERE i=false ORDER BY i;
diff --git a/src/backend/optimizer/util/predtest.c b/src/backend/optimizer/util/predtest.c
index 182d5b1523..90701dab08 100644
--- a/src/backend/optimizer/util/predtest.c
+++ b/src/backend/optimizer/util/predtest.c
@@ -15,6 +15,7 @@
  */
 #include "postgres.h"

+#include "catalog/pg_operator.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_type.h"
 #include "executor/executor.h"
@@ -1093,7 +1094,7 @@ arrayexpr_cleanup_fn(PredIterInfo info)
  *
  * We return true if able to prove the implication, false if not.
  *
- * We have three strategies for determining whether one simple clause
+ * We have several strategies for determining whether one simple clause
  * implies another:
  *
  * A simple and general way is to see if they are equal(); this works for any
@@ -1101,6 +1102,12 @@ arrayexpr_cleanup_fn(PredIterInfo info)
  * there is an implied assumption that the functions in the expression are
  * immutable --- but this was checked for the predicate by the caller.)
  *
+ * Another way that always works is that "x = TRUE" implies "x" (and vice
+ * versa), likewise for "x = FALSE" and "NOT x".  These can be worth
+ * checking because, while we preferentially simplify boolean comparisons
+ * down to "x" and "NOT x", the other form has to be dealt with anyway
+ * in the context of index conditions.
+ *
  * If the predicate is of the form "foo IS NOT NULL", and we are considering
  * strong implication, we can conclude that the predicate is implied if the
  * clause is strict for "foo", i.e., it must yield false or NULL when "foo"
@@ -1124,6 +1131,43 @@ predicate_implied_by_simple_clause(Expr *predicate, Node *clause,
     if (equal((Node *) predicate, clause))
         return true;

+    /* Next see if clause is boolean equality to a constant */
+    if (is_opclause(clause) &&
+        ((OpExpr *) clause)->opno == BooleanEqualOperator)
+    {
+        OpExpr       *op = (OpExpr *) clause;
+        Node       *rightop;
+
+        Assert(list_length(op->args) == 2);
+        rightop = lsecond(op->args);
+        /* We might never see a null Const here, but better check anyway */
+        if (rightop && IsA(rightop, Const) &&
+            !((Const *) rightop)->constisnull)
+        {
+            Node       *leftop = linitial(op->args);
+
+            if (DatumGetBool(((Const *) rightop)->constvalue))
+            {
+                /* X = true implies X */
+                if (equal(predicate, leftop))
+                    return true;
+            }
+            else
+            {
+                /* X = false implies NOT X */
+                if (is_notclause(predicate) &&
+                    equal(get_notclausearg(predicate), leftop))
+                    return true;
+            }
+        }
+    }
+
+    /*
+     * We could likewise check whether the predicate is boolean equality to a
+     * constant; but there are no known use-cases for that at the moment,
+     * assuming that the predicate has been through const-folding.
+     */
+
     /* Next try the IS NOT NULL case */
     if (!weak &&
         predicate && IsA(predicate, NullTest))