From: Tom Lane <tgl@sss.pgh.pa.us>
To: Richard Guo <guofenglinux@gmail.com>
Cc: Jeff Janes <jeff.janes@gmail.com>
Cc: sindysenorita@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17618: unnecessary filter column <> text even after adding index
Date: Mon, 07 Nov 2022 16:06:33 -0500
Message-ID: <3495354.1667855193@sss.pgh.pa.us> (raw)
In-Reply-To: <CAMbWs48DuHbqtAgMjpj5Jq_ubxR0+SS-urLuVi=9Dx5FTkJieA@mail.gmail.com>
References: <17618-7a2240bfaa7e84ae@postgresql.org>
<349136.1663601052@sss.pgh.pa.us>
<CAMkU=1wqvsMwnQTrQ0zoRo2AkYhuEY-RuzpaXrvz7iNu38CVqg@mail.gmail.com>
<739953.1663701274@sss.pgh.pa.us>
<946195.1663718159@sss.pgh.pa.us>
<CAMbWs4-o-VLk3gXqz7W7ZS384S5mvW5noB8f_BKzU_c77DBjXg@mail.gmail.com>
<2100290.1663942209@sss.pgh.pa.us>
<CAMbWs4-5CtCGx_nbzfPFTtDCXjmtCOR3vb-OfK4eNEAKwLQPSQ@mail.gmail.com>
<2174559.1663977891@sss.pgh.pa.us>
<CAMbWs498iR-5gVbsS0DCmoeK4YNs0nny6xmW-eLvm=C-KXm-Ag@mail.gmail.com>
<CAMbWs4_AmLq9ssUh+fu8KZk8QPhx21=+o3whspLNAhxY6T0Xzw@mail.gmail.com>
<CAMbWs49oBk4EfHC+44xkuAwQYnGH5pyKP40=FwZv9Zecx9=3Mg@mail.gmail.com>
<2616649.1667668064@sss.pgh.pa.us>
<CAMbWs48DuHbqtAgMjpj5Jq_ubxR0+SS-urLuVi=9Dx5FTkJieA@mail.gmail.com>
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
Attachments:v5-0001-constant-folding-for-indexquals-in-bitmap-scan.patch (text/x-diff, 4.4K) downloaddiff --git a/contrib/btree_gin/expected/bool.out b/contrib/btree_gin/expected/bool.outindex 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.sqlindex 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.cindex 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))
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, guofenglinux@gmail.com, jeff.janes@gmail.com, sindysenorita@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17618: unnecessary filter column <> text even after adding index
In-Reply-To: <3495354.1667855193@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox