pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17618: unnecessary filter column <> text even after adding index
21+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2022-11-08 042c9091f0 Produce more-optimal plans for bitmap scans on boolean columns.
* BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-19 14:28 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 2 replies; 21+ messages in thread
From: PG Bug reporting form @ 2022-09-19 14:28 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: sindysenorita@gmail.com
The following bug has been logged on the website:
Bug reference: 17618
Logged by: Sindy Senorita
Email address: sindysenorita@gmail.com
PostgreSQL version: 13.7
Operating system: Ubuntu
Description:
Hi, I'm not sure if this is a bug or feature, but definitely not what I've
expected
So I have a table with "status" column which can contains 'valid',
'invalid', 'pending', 'unknown'.
A very simple table
CREATE TABLE public.test (
id varchar NOT NULL,
status varchar NOT NULL,
CONSTRAINT test__pkey PRIMARY KEY (id)
)
CREATE INDEX pending_test_4 ON public.test USING btree ((((status)::text <>
'invalid'::text)));
notice that I've created an index to guide statuses that is not 'invalid
my query is:
SELECT * FROM test WHERE status != 'invalid'
When I run explain analyze on that with SET enable_seqscan = off, I got
QUERY PLAN
|
------------------------------------------------------------------------------------------------------------------------+
Bitmap Heap Scan on test (cost=4.62..8.37 rows=120 width=160) (actual
time=0.088..0.134 rows=117 loops=1) |
Filter: ((status)::text <> 'invalid'::text)
|
Heap Blocks: exact=3
|
-> Bitmap Index Scan on pending_test_4 (cost=0.00..4.59 rows=60 width=0)
(actual time=0.073..0.073 rows=117 loops=1)|
Index Cond: (((status)::text <> 'invalid'::text) = true)
|
Planning Time: 0.222 ms
|
Execution Time: 0.172 ms
|
The plan has used the index condition just right, but it still perform
aditional bitmap heap scan just to filter for a clause that exactly match
the index. And worse, it double the query cost
My questions are:
1. Is this a bug? or intended feature by design? If it is by design, I'd be
very happy to learn the rationale behind it.
2. Is there any way to skip/avoid the additional bitmap scan?
3. Could there be a better solution for my query. Suppose that the variants
of the status is unknown so query SELECT .. WHERE STATUS IN (all status
beside 'invalid') is not possible
Many thanks!
Sindy
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-19 15:24 Tom Lane <tgl@sss.pgh.pa.us>
parent: PG Bug reporting form <noreply@postgresql.org>
1 sibling, 2 replies; 21+ messages in thread
From: Tom Lane @ 2022-09-19 15:24 UTC (permalink / raw)
To: sindysenorita@gmail.com; +Cc: pgsql-bugs@lists.postgresql.org
PG Bug reporting form <noreply@postgresql.org> writes:
> When I run explain analyze on that with SET enable_seqscan = off, I got
> QUERY PLAN
> |
> ------------------------------------------------------------------------------------------------------------------------+
> Bitmap Heap Scan on test (cost=4.62..8.37 rows=120 width=160) (actual
> time=0.088..0.134 rows=117 loops=1) |
> Filter: ((status)::text <> 'invalid'::text)
> |
> Heap Blocks: exact=3
> |
> -> Bitmap Index Scan on pending_test_4 (cost=0.00..4.59 rows=60 width=0)
> (actual time=0.073..0.073 rows=117 loops=1)|
> Index Cond: (((status)::text <> 'invalid'::text) = true)
> |
> Planning Time: 0.222 ms
> |
> Execution Time: 0.172 ms
> |
This is exactly what is expected; it's not a bug.
> The plan has used the index condition just right, but it still perform
> aditional bitmap heap scan just to filter for a clause that exactly match
> the index. And worse, it double the query cost
The filter condition is required because the bitmap produced by the index
can be lossy, ie it might identify more rows than actually satisfy the
condition. BitmapHeapNext will only actually apply the condition if
the index reports that that happened, so in practice for this sort of
query the filter condition probably never gets rechecked.
The "doubled cost" has nothing whatever to do with the filter condition;
most of that is concerned with the number of disk pages touched. It
might help you to read
https://www.postgresql.org/docs/current/using-explain.html
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-19 15:45 Sindy Senorita <sindysenorita@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 21+ messages in thread
From: Sindy Senorita @ 2022-09-19 15:45 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org
I see, quick google search takes me to BitmapHeapNext implementation here
https://doxygen.postgresql.org/nodeBitmapHeapscan_8c_source.html#l00072. I
hope this is what you mean
Noted. Thanks for the explanation
Cheers
On Mon, Sep 19, 2022 at 10:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
> > When I run explain analyze on that with SET enable_seqscan = off, I got
> > QUERY PLAN
>
> > |
> >
> ------------------------------------------------------------------------------------------------------------------------+
> > Bitmap Heap Scan on test (cost=4.62..8.37 rows=120 width=160) (actual
> > time=0.088..0.134 rows=117 loops=1) |
> > Filter: ((status)::text <> 'invalid'::text)
>
> > |
> > Heap Blocks: exact=3
>
> > |
> > -> Bitmap Index Scan on pending_test_4 (cost=0.00..4.59 rows=60
> width=0)
> > (actual time=0.073..0.073 rows=117 loops=1)|
> > Index Cond: (((status)::text <> 'invalid'::text) = true)
>
> > |
> > Planning Time: 0.222 ms
>
> > |
> > Execution Time: 0.172 ms
>
> > |
>
>
> This is exactly what is expected; it's not a bug.
>
> > The plan has used the index condition just right, but it still perform
> > aditional bitmap heap scan just to filter for a clause that exactly match
> > the index. And worse, it double the query cost
>
> The filter condition is required because the bitmap produced by the index
> can be lossy, ie it might identify more rows than actually satisfy the
> condition. BitmapHeapNext will only actually apply the condition if
> the index reports that that happened, so in practice for this sort of
> query the filter condition probably never gets rechecked.
>
> The "doubled cost" has nothing whatever to do with the filter condition;
> most of that is concerned with the number of disk pages touched. It
> might help you to read
>
> https://www.postgresql.org/docs/current/using-explain.html
>
> regards, tom lane
>
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-19 15:53 David G. Johnston <david.g.johnston@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
1 sibling, 0 replies; 21+ messages in thread
From: David G. Johnston @ 2022-09-19 15:53 UTC (permalink / raw)
To: sindysenorita@gmail.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Mon, Sep 19, 2022 at 8:15 AM PG Bug reporting form <
noreply@postgresql.org> wrote:
>
> CREATE TABLE public.test (
> id varchar NOT NULL,
> status varchar NOT NULL,
> CONSTRAINT test__pkey PRIMARY KEY (id)
> )
>
> CREATE INDEX pending_test_4 ON public.test USING btree ((((status)::text <>
> 'invalid'::text)));
>
> notice that I've created an index to guide statuses that is not 'invalid
> my query is:
> SELECT * FROM test WHERE status != 'invalid'
>
Your index contains none of the fields in the original table so the system
can never answer your inquiry using only the index.
You may find this to be informative:
https://www.postgresql.org/docs/devel/indexes-index-only-scans.html
Usually on a "status" field doing a few partial indexes gets you the best
result. The more statuses you need to be concerned about the more likely
just scanning the table is going to win out in performance. But if you do
only care about a few the smaller index size will be of benefit to keep
them in memory. A covering index may be of use as well though for rapidly
changing statuses tuple visibility is going to be a challenge. In short,
you seem to be providing a non-real situation and asking for advice that is
situational in nature.
David J.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-20 17:12 Jeff Janes <jeff.janes@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 21+ messages in thread
From: Jeff Janes @ 2022-09-20 17:12 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, Sep 19, 2022 at 11:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> > The plan has used the index condition just right, but it still perform
> > aditional bitmap heap scan just to filter for a clause that exactly match
> > the index. And worse, it double the query cost
>
> The filter condition is required because the bitmap produced by the index
> can be lossy, ie it might identify more rows than actually satisfy the
> condition. BitmapHeapNext will only actually apply the condition if
> the index reports that that happened, so in practice for this sort of
> query the filter condition probably never gets rechecked.
>
You are describing a Recheck, but attributing its properties to a Filter.
I think that the filter condition always gets checked. It does so if I
create a tattler function which raises a notice every time it is called and
then build an index on a Boolean expression over that function (but of
course that inevitably does change the code paths a bit).
I don't know about being a bug, but it is at least a mild mal-feature that
boolean index columns/expressions can't be dealt with better, and have to
be handed in a filter rather than in a recheck.
Cheers,
Jeff
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-20 19:14 Tom Lane <tgl@sss.pgh.pa.us>
parent: Jeff Janes <jeff.janes@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Tom Lane @ 2022-09-20 19:14 UTC (permalink / raw)
To: Jeff Janes <jeff.janes@gmail.com>; +Cc: sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
Jeff Janes <jeff.janes@gmail.com> writes:
> You are describing a Recheck, but attributing its properties to a Filter.
You're right of course, momentary brain fade on my part.
> I don't know about being a bug, but it is at least a mild mal-feature that
> boolean index columns/expressions can't be dealt with better, and have to
> be handed in a filter rather than in a recheck.
Yeah ... looking at create_bitmap_scan_plan, I see that it does this:
/*
* When dealing with special operators, we will at this point have
* duplicate clauses in qpqual and bitmapqualorig. We may as well drop
* 'em from bitmapqualorig, since there's no point in making the tests
* twice.
*/
bitmapqualorig = list_difference_ptr(bitmapqualorig, qpqual);
I wonder if that isn't backwards, ie we should prefer to put duplicates
in bitmapqualorig (the recheck condition) instead of qpqual (the filter).
If my head is screwed on correctly today, that should allow us to skip
checking the condition much of the time, and the skip would be safe
if the index is correctly asserting that no recheck is needed.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-20 23:55 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 21+ messages in thread
From: Tom Lane @ 2022-09-20 23:55 UTC (permalink / raw)
To: Jeff Janes <jeff.janes@gmail.com>; +Cc: sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
I wrote:
> I wonder if that isn't backwards, ie we should prefer to put duplicates
> in bitmapqualorig (the recheck condition) instead of qpqual (the filter).
> If my head is screwed on correctly today, that should allow us to skip
> checking the condition much of the time, and the skip would be safe
> if the index is correctly asserting that no recheck is needed.
Flipping the removal around has the effect I expected on the plan shape,
but some of the regression test queries now give the wrong answer, so
there's something faulty about that analysis.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-23 13:29 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 2 replies; 21+ messages in thread
From: Richard Guo @ 2022-09-23 13:29 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Janes <jeff.janes@gmail.com>; sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Sep 21, 2022 at 7:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Flipping the removal around has the effect I expected on the plan shape,
> but some of the regression test queries now give the wrong answer, so
> there's something faulty about that analysis.
I think we may have a minor mistake when constructing the qpqual list in
create_bitmap_scan_plan. The qpqual list is supposed to be scan_clauses
minus indexquals. So we check each scan clause to see if it is redundant
with any indexqual, by using equal, checking EC or using
predicate_implied_by.
Note that the indexqual here may not be the form that has been going
through constant folding. Such as in this case with a boolean index, the
indexqual would be converted to 'indexkey expression = TRUE' by
match_boolean_index_clause. And that may make us fail to tell the scan
clause is redundant.
The comment of predicate_implied_by() says
* The top-level List structure of each list corresponds to an AND list.
* We assume that eval_const_expressions() has been applied and so there
* are no un-flattened ANDs or ORs (e.g., no AND immediately within an AND,
* including AND just below the top-level List structure).
So I think we need to run eval_const_expressions on indexquals before we
check for duplicate clauses, something like attached.
Thanks
Richard
Attachments:
v1-0001-constant-folding-for-indexquals-in-bitmap-scan.patch (application/octet-stream, 1015B) download
From f1197e4d276e4fcfa16e3bfb01051c2e86940ef7 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Fri, 23 Sep 2022 21:13:15 +0800
Subject: [PATCH v1] constant folding for indexquals in bitmap scan
---
src/backend/optimizer/plan/createplan.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index ab4d8e201d..b201409969 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -3244,6 +3244,12 @@ create_bitmap_scan_plan(PlannerInfo *root,
RestrictInfo *rinfo = lfirst_node(RestrictInfo, l);
Node *clause = (Node *) rinfo->clause;
+ /*
+ * Do constant folding for indexquals before checking for duplicate
+ * clauses.
+ */
+ indexquals = (List *) eval_const_expressions(root, (Node *) indexquals);
+
if (rinfo->pseudoconstant)
continue; /* we may drop pseudoconstants here */
if (list_member(indexquals, clause))
--
2.31.0
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-23 14:10 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
1 sibling, 1 reply; 21+ messages in thread
From: Tom Lane @ 2022-09-23 14:10 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Jeff Janes <jeff.janes@gmail.com>; sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
Richard Guo <guofenglinux@gmail.com> writes:
> So I think we need to run eval_const_expressions on indexquals before we
> check for duplicate clauses, something like attached.
[ squint... ] Surely that was done long before we ever get here?
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-23 23:54 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 21+ messages in thread
From: Richard Guo @ 2022-09-23 23:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Janes <jeff.janes@gmail.com>; sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Sep 23, 2022 at 10:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Richard Guo <guofenglinux@gmail.com> writes:
> > So I think we need to run eval_const_expressions on indexquals before we
> > check for duplicate clauses, something like attached.
>
> [ squint... ] Surely that was done long before we ever get here?
We should have already done that long before. It seems afterwards we may
do additional transformation on indexquals. In this case with a boolean
index, I can see we convert the indexqual to form 'indexkey = TRUE' in
match_boolean_index_clause.
Thanks
Richard
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-24 00:04 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Tom Lane @ 2022-09-24 00:04 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Jeff Janes <jeff.janes@gmail.com>; sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
Richard Guo <guofenglinux@gmail.com> writes:
> On Fri, Sep 23, 2022 at 10:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Richard Guo <guofenglinux@gmail.com> writes:
>>> So I think we need to run eval_const_expressions on indexquals before we
>>> check for duplicate clauses, something like attached.
>> [ squint... ] Surely that was done long before we ever get here?
> We should have already done that long before. It seems afterwards we may
> do additional transformation on indexquals. In this case with a boolean
> index, I can see we convert the indexqual to form 'indexkey = TRUE' in
> match_boolean_index_clause.
Of course, but what about that transformation would introduce something
that eval_const_expressions could simplify? (Actually, now that I think
about it, I think eval_const_expressions would break it completely because
it'd re-canonicalize the expression as just 'indexkey', exactly what we
don't want here.) In any case, if there's something between the
eval_const_expressions pass and createplan.c that introduces simplifiable
expressions, I think it's on that something's head to re-simplify; we
don't want to do something so expensive in a main code path if it's
usually going to be a complete waste.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-24 00:06 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
1 sibling, 0 replies; 21+ messages in thread
From: Richard Guo @ 2022-09-24 00:06 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Janes <jeff.janes@gmail.com>; sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Sep 23, 2022 at 9:29 PM Richard Guo <guofenglinux@gmail.com> wrote:
> So I think we need to run eval_const_expressions on indexquals before we
> check for duplicate clauses, something like attached.
>
BTW, (revise to the v1 patch), if this is the right way to go, we should
do that before the foreach loop, so that we need to run
eval_const_expressions on indexquals only once rather than for each scan
clause.
Thanks
Richard
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-24 00:41 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 21+ messages in thread
From: Richard Guo @ 2022-09-24 00:41 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Janes <jeff.janes@gmail.com>; sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
On Sat, Sep 24, 2022 at 8:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Richard Guo <guofenglinux@gmail.com> writes:
> > We should have already done that long before. It seems afterwards we may
> > do additional transformation on indexquals. In this case with a boolean
> > index, I can see we convert the indexqual to form 'indexkey = TRUE' in
> > match_boolean_index_clause.
>
> Of course, but what about that transformation would introduce something
> that eval_const_expressions could simplify? (Actually, now that I think
> about it, I think eval_const_expressions would break it completely because
> it'd re-canonicalize the expression as just 'indexkey', exactly what we
> don't want here.) In any case, if there's something between the
> eval_const_expressions pass and createplan.c that introduces simplifiable
> expressions, I think it's on that something's head to re-simplify; we
> don't want to do something so expensive in a main code path if it's
> usually going to be a complete waste.
Yeah, I agree that running eval_const_expressions here is expensive.
Maybe we can just do the reverse transformation in
create_bitmap_scan_plan against what we do for boolean index in
match_boolean_index_clause?
I think it's necessary to re-simplify the indexquals here, otherwise we
may fail to compare scan_clauses to indexquals correctly.
Thanks
Richard
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-09-26 11:42 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Richard Guo @ 2022-09-26 11:42 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Janes <jeff.janes@gmail.com>; sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
On Sat, Sep 24, 2022 at 8:41 AM Richard Guo <guofenglinux@gmail.com> wrote:
>
> On Sat, Sep 24, 2022 at 8:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> Richard Guo <guofenglinux@gmail.com> writes:
>> > We should have already done that long before. It seems afterwards we may
>> > do additional transformation on indexquals. In this case with a boolean
>> > index, I can see we convert the indexqual to form 'indexkey = TRUE' in
>> > match_boolean_index_clause.
>>
>> Of course, but what about that transformation would introduce something
>> that eval_const_expressions could simplify? (Actually, now that I think
>> about it, I think eval_const_expressions would break it completely because
>> it'd re-canonicalize the expression as just 'indexkey', exactly what we
>> don't want here.) In any case, if there's something between the
>> eval_const_expressions pass and createplan.c that introduces simplifiable
>> expressions, I think it's on that something's head to re-simplify; we
>> don't want to do something so expensive in a main code path if it's
>> usually going to be a complete waste.
>
>
> Yeah, I agree that running eval_const_expressions here is expensive.
> Maybe we can just do the reverse transformation in
> create_bitmap_scan_plan against what we do for boolean index in
> match_boolean_index_clause?
>
Following this idea, I come up with v2 patch. Is this the right
direction to go?
Thanks
Richard
Attachments:
v2-0001-constant-folding-for-indexquals-in-bitmap-scan.patch (application/octet-stream, 4.4K) download
From 65cbc7183463183878973924facb24266c7cbd9d Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Mon, 26 Sep 2022 19:07:42 +0800
Subject: [PATCH v2] constant folding for indexquals in bitmap scan
---
src/backend/optimizer/plan/createplan.c | 31 +++++++++++++++++++++++--
src/backend/optimizer/util/clauses.c | 13 ++++++-----
src/include/optimizer/optimizer.h | 2 ++
3 files changed, 38 insertions(+), 8 deletions(-)
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index ab4d8e201d..6b54b2129e 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -20,6 +20,7 @@
#include "access/sysattr.h"
#include "catalog/pg_class.h"
+#include "catalog/pg_operator.h"
#include "foreign/fdwapi.h"
#include "miscadmin.h"
#include "nodes/extensible.h"
@@ -3480,11 +3481,37 @@ create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual,
{
IndexClause *iclause = (IndexClause *) lfirst(l);
RestrictInfo *rinfo = iclause->rinfo;
+ ListCell *l2;
Assert(!rinfo->pseudoconstant);
subquals = lappend(subquals, rinfo->clause);
- subindexquals = list_concat(subindexquals,
- get_actual_clauses(iclause->indexquals));
+
+ /*
+ * The indexquals may have been transformed into form "indexkey =
+ * true/false" in match_boolean_index_clause(). We need to
+ * re-simplify them in order to compare them to scan_clauses in
+ * create_bitmap_scan_plan().
+ */
+ foreach(l2, iclause->indexquals)
+ {
+ RestrictInfo *restrictinfo = lfirst_node(RestrictInfo, l2);
+ OpExpr *clause = (OpExpr *) restrictinfo->clause;
+
+ Assert(!restrictinfo->pseudoconstant);
+
+ if (is_opclause(clause) &&
+ clause->opno == BooleanEqualOperator)
+ {
+ Node *simple;
+
+ simple = simplify_boolean_equality(clause->opno,
+ clause->args);
+ if (simple) /* successfully simplified it */
+ clause = (OpExpr *) simple;
+ }
+
+ subindexquals = lappend(subindexquals, (Node *) clause);
+ }
if (rinfo->parent_ec)
subindexECs = lappend(subindexECs, rinfo->parent_ec);
}
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index bf3a7cae60..cb3d0d2523 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -119,7 +119,6 @@ static List *simplify_or_arguments(List *args,
static List *simplify_and_arguments(List *args,
eval_const_expressions_context *context,
bool *haveNull, bool *forceFalse);
-static Node *simplify_boolean_equality(Oid opno, List *args);
static Expr *simplify_function(Oid funcid,
Oid result_type, int32 result_typmod,
Oid result_collid, Oid input_collid, List **args_p,
@@ -3774,8 +3773,8 @@ simplify_and_arguments(List *args,
}
/*
- * Subroutine for eval_const_expressions: try to simplify boolean equality
- * or inequality condition
+ * simplify_boolean_equality: try to simplify boolean equality or inequality
+ * condition
*
* Inputs are the operator OID and the simplified arguments to the operator.
* Returns a simplified expression if successful, or NULL if cannot
@@ -3787,10 +3786,12 @@ simplify_and_arguments(List *args,
* ensures that we will recognize these forms as being equivalent in, for
* example, partial index matching.
*
- * We come here only if simplify_function has failed; therefore we cannot
- * see two constant inputs, nor a constant-NULL input.
+ * We come here only if simplify_function has failed in
+ * eval_const_expressions_mutator, or we need to re-simplify the boolean
+ * equality expression in create_bitmap_subplan; therefore we cannot see two
+ * constant inputs, nor a constant-NULL input.
*/
-static Node *
+Node *
simplify_boolean_equality(Oid opno, List *args)
{
Node *leftop;
diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h
index 409005bae9..e5d110744b 100644
--- a/src/include/optimizer/optimizer.h
+++ b/src/include/optimizer/optimizer.h
@@ -150,6 +150,8 @@ extern Node *estimate_expression_value(PlannerInfo *root, Node *node);
extern Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
Oid result_collation);
+extern Node *simplify_boolean_equality(Oid opno, List *args);
+
extern List *expand_function_arguments(List *args, bool include_out_arguments,
Oid result_type,
struct HeapTupleData *func_tuple);
--
2.31.0
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-11-02 07:46 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Richard Guo @ 2022-11-02 07:46 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Janes <jeff.janes@gmail.com>; sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, Sep 26, 2022 at 7:42 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Sat, Sep 24, 2022 at 8:41 AM Richard Guo <guofenglinux@gmail.com>
> wrote:
>
>> Yeah, I agree that running eval_const_expressions here is expensive.
>> Maybe we can just do the reverse transformation in
>> create_bitmap_scan_plan against what we do for boolean index in
>> match_boolean_index_clause?
>>
>
> Following this idea, I come up with v2 patch. Is this the right
> direction to go?
>
Update with v3 patch, nothing changes except fixes a test failure
spotted by cfbot.
Thanks
Richard
Attachments:
v3-0001-constant-folding-for-indexquals-in-bitmap-scan.patch (application/octet-stream, 5.0K) download
From fa5550937c15c2da9b302aaebef24458d93b532b Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Wed, 2 Nov 2022 15:06:46 +0800
Subject: [PATCH v3] constant folding for indexquals in bitmap scan
---
contrib/btree_gin/expected/bool.out | 2 +-
src/backend/optimizer/plan/createplan.c | 31 +++++++++++++++++++++++--
src/backend/optimizer/util/clauses.c | 13 ++++++-----
src/include/optimizer/optimizer.h | 2 ++
4 files changed, 39 insertions(+), 9 deletions(-)
diff --git a/contrib/btree_gin/expected/bool.out b/contrib/btree_gin/expected/bool.out
index 207a3f2328..51a70eb530 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)
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index ac86ce9003..10705cb210 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -20,6 +20,7 @@
#include "access/sysattr.h"
#include "catalog/pg_class.h"
+#include "catalog/pg_operator.h"
#include "foreign/fdwapi.h"
#include "miscadmin.h"
#include "nodes/extensible.h"
@@ -3480,11 +3481,37 @@ create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual,
{
IndexClause *iclause = (IndexClause *) lfirst(l);
RestrictInfo *rinfo = iclause->rinfo;
+ ListCell *l2;
Assert(!rinfo->pseudoconstant);
subquals = lappend(subquals, rinfo->clause);
- subindexquals = list_concat(subindexquals,
- get_actual_clauses(iclause->indexquals));
+
+ /*
+ * The indexquals may have been transformed into form "indexkey =
+ * true/false" in match_boolean_index_clause(). We need to
+ * re-simplify them in order to compare them to scan_clauses in
+ * create_bitmap_scan_plan().
+ */
+ foreach(l2, iclause->indexquals)
+ {
+ RestrictInfo *restrictinfo = lfirst_node(RestrictInfo, l2);
+ OpExpr *clause = (OpExpr *) restrictinfo->clause;
+
+ Assert(!restrictinfo->pseudoconstant);
+
+ if (is_opclause(clause) &&
+ clause->opno == BooleanEqualOperator)
+ {
+ Node *simple;
+
+ simple = simplify_boolean_equality(clause->opno,
+ clause->args);
+ if (simple) /* successfully simplified it */
+ clause = (OpExpr *) simple;
+ }
+
+ subindexquals = lappend(subindexquals, (Node *) clause);
+ }
if (rinfo->parent_ec)
subindexECs = lappend(subindexECs, rinfo->parent_ec);
}
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 7fb32a0710..50cd0cbe79 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -119,7 +119,6 @@ static List *simplify_or_arguments(List *args,
static List *simplify_and_arguments(List *args,
eval_const_expressions_context *context,
bool *haveNull, bool *forceFalse);
-static Node *simplify_boolean_equality(Oid opno, List *args);
static Expr *simplify_function(Oid funcid,
Oid result_type, int32 result_typmod,
Oid result_collid, Oid input_collid, List **args_p,
@@ -3774,8 +3773,8 @@ simplify_and_arguments(List *args,
}
/*
- * Subroutine for eval_const_expressions: try to simplify boolean equality
- * or inequality condition
+ * simplify_boolean_equality: try to simplify boolean equality or inequality
+ * condition
*
* Inputs are the operator OID and the simplified arguments to the operator.
* Returns a simplified expression if successful, or NULL if cannot
@@ -3787,10 +3786,12 @@ simplify_and_arguments(List *args,
* ensures that we will recognize these forms as being equivalent in, for
* example, partial index matching.
*
- * We come here only if simplify_function has failed; therefore we cannot
- * see two constant inputs, nor a constant-NULL input.
+ * We come here only if simplify_function has failed in
+ * eval_const_expressions_mutator, or we need to re-simplify the boolean
+ * equality expression in create_bitmap_subplan; therefore we cannot see two
+ * constant inputs, nor a constant-NULL input.
*/
-static Node *
+Node *
simplify_boolean_equality(Oid opno, List *args)
{
Node *leftop;
diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h
index 409005bae9..e5d110744b 100644
--- a/src/include/optimizer/optimizer.h
+++ b/src/include/optimizer/optimizer.h
@@ -150,6 +150,8 @@ extern Node *estimate_expression_value(PlannerInfo *root, Node *node);
extern Expr *evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
Oid result_collation);
+extern Node *simplify_boolean_equality(Oid opno, List *args);
+
extern List *expand_function_arguments(List *args, bool include_out_arguments,
Oid result_type,
struct HeapTupleData *func_tuple);
--
2.31.0
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-11-05 17:07 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Tom Lane @ 2022-11-05 17:07 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Jeff Janes <jeff.janes@gmail.com>; sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
Richard Guo <guofenglinux@gmail.com> writes:
> Update with v3 patch, nothing changes except fixes a test failure
> spotted by cfbot.
I think this is pretty close to usable, except that I don't believe
reusing simplify_boolean_equality this way is a great idea.
It does more than we need (surely the LHS-is-Const case cannot occur here)
and it has assumptions that I'm not sure hold --- particularly the bit
about !constisnull. I'd be inclined to just copy-and-paste the three or
four lines we need.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-11-05 17:23 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 21+ messages in thread
From: Tom Lane @ 2022-11-05 17:23 UTC (permalink / raw)
To: Jeff Janes <jeff.janes@gmail.com>; +Cc: sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
I wrote:
>> I wonder if that isn't backwards, ie we should prefer to put duplicates
>> in bitmapqualorig (the recheck condition) instead of qpqual (the filter).
>> If my head is screwed on correctly today, that should allow us to skip
>> checking the condition much of the time, and the skip would be safe
>> if the index is correctly asserting that no recheck is needed.
> Flipping the removal around has the effect I expected on the plan shape,
> but some of the regression test queries now give the wrong answer, so
> there's something faulty about that analysis.
BTW, after looking more closely I see my mistake. An example of the
sort of plan that fails with that change is
Sort
Sort Key: proname
-> Bitmap Heap Scan on pg_proc
- Filter: (proname ~~ 'RI\_FKey%del'::text)
+ Recheck Cond: (proname ~~ 'RI\_FKey%del'::text)
-> Bitmap Index Scan on pg_proc_proname_args_nsp_index
Index Cond: ((proname >= 'RI_FKey'::text) AND (proname < 'RI_FKez'::text))
(6 rows)
The difficulty here is pretty obvious: the original clause is stricter
than the index conditions generated from it. So even if the index
enforces the index conditions exactly, we still need to check the
original clause, and so it can't be relegated to the recheck field.
To improve this, we'd need to track which elements of bitmapqualorig
correspond exactly to index conditions, which we don't do ATM.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-11-07 09:01 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 21+ messages in thread
From: Richard Guo @ 2022-11-07 09:01 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Janes <jeff.janes@gmail.com>; sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
On Sun, Nov 6, 2022 at 1:07 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Richard Guo <guofenglinux@gmail.com> writes:
> > Update with v3 patch, nothing changes except fixes a test failure
> > spotted by cfbot.
>
> I think this is pretty close to usable, except that I don't believe
> reusing simplify_boolean_equality this way is a great idea.
> It does more than we need (surely the LHS-is-Const case cannot occur here)
> and it has assumptions that I'm not sure hold --- particularly the bit
> about !constisnull. I'd be inclined to just copy-and-paste the three or
> four lines we need.
Thanks for the suggestion. Yes, simplify_boolean_equality is doing more
than we need. I think here we just intend to handle indexquals of form
"indexkey = true/false", which seems can only come out from function
match_boolean_index_clause. From what this function does, we are sure
the constant input can be only on right (as you pointed out), and the
operator can only be BooleanEqualOperator. Also it seems the assumption
about !constisnull holds, as match_boolean_index_clause would not make a
clause with a constant-NULL input.
I've updated the patch according to the suggestions as in v4. Thanks
for reviewing this patch!
Thanks
Richard
Attachments:
v4-0001-constant-folding-for-indexquals-in-bitmap-scan.patch (application/octet-stream, 3.1K) download
From 17e63c3aaa5df849634a2770fdf18a0e88e56b08 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Mon, 7 Nov 2022 15:56:46 +0800
Subject: [PATCH v4] constant folding for indexquals in bitmap scan
---
contrib/btree_gin/expected/bool.out | 2 +-
src/backend/optimizer/plan/createplan.c | 47 +++++++++++++++++++++++--
2 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/contrib/btree_gin/expected/bool.out b/contrib/btree_gin/expected/bool.out
index 207a3f2328..51a70eb530 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)
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index ac86ce9003..ef50cf8b18 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -20,6 +20,7 @@
#include "access/sysattr.h"
#include "catalog/pg_class.h"
+#include "catalog/pg_operator.h"
#include "foreign/fdwapi.h"
#include "miscadmin.h"
#include "nodes/extensible.h"
@@ -3480,11 +3481,53 @@ create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual,
{
IndexClause *iclause = (IndexClause *) lfirst(l);
RestrictInfo *rinfo = iclause->rinfo;
+ ListCell *l2;
Assert(!rinfo->pseudoconstant);
subquals = lappend(subquals, rinfo->clause);
- subindexquals = list_concat(subindexquals,
- get_actual_clauses(iclause->indexquals));
+
+ /*
+ * The indexquals may have been transformed into form "indexkey =
+ * true/false" in match_boolean_index_clause. We need to
+ * re-simplify them in order to compare them to scan_clauses in
+ * create_bitmap_scan_plan.
+ *
+ * According to what match_boolean_index_clause does, the constant
+ * input can be only on right, and it cannot be a constant-NULL.
+ */
+ foreach(l2, iclause->indexquals)
+ {
+ RestrictInfo *restrictinfo = lfirst_node(RestrictInfo, l2);
+ Node *clause = (Node *) restrictinfo->clause;
+
+ Assert(!restrictinfo->pseudoconstant);
+
+ if (is_opclause(clause) &&
+ ((OpExpr *) clause)->opno == BooleanEqualOperator)
+ {
+ Node *rightop;
+
+ Assert(list_length(((OpExpr *) clause)->args) == 2);
+ rightop = lsecond(((OpExpr *) clause)->args);
+
+ /*
+ * The constant input can be only on right, and it cannot
+ * be a constant-NULL.
+ */
+ if (rightop && IsA(rightop, Const))
+ {
+ Node *leftop = linitial(((OpExpr *) clause)->args);
+
+ Assert(!((Const *) rightop)->constisnull);
+ if (DatumGetBool(((Const *) rightop)->constvalue))
+ clause = leftop; /* indexkey = true */
+ else
+ clause = negate_clause(leftop); /* indexkey = false */
+ }
+ }
+
+ subindexquals = lappend(subindexquals, clause);
+ }
if (rinfo->parent_ec)
subindexECs = lappend(subindexECs, rinfo->parent_ec);
}
--
2.31.0
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-11-07 21:06 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Tom Lane @ 2022-11-07 21:06 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Jeff Janes <jeff.janes@gmail.com>; sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
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) download
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))
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-11-08 01:59 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 21+ messages in thread
From: Richard Guo @ 2022-11-08 01:59 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Janes <jeff.janes@gmail.com>; sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, Nov 8, 2022 at 5:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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?
It works for me. predtest.c is a more common place so that there may be
other cases that can benefit from this change. Thanks for the new
patch!
Thanks
Richard
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #17618: unnecessary filter column <> text even after adding index
@ 2022-11-08 15:37 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 0 replies; 21+ messages in thread
From: Tom Lane @ 2022-11-08 15:37 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Jeff Janes <jeff.janes@gmail.com>; sindysenorita@gmail.com; pgsql-bugs@lists.postgresql.org
Richard Guo <guofenglinux@gmail.com> writes:
> It works for me. predtest.c is a more common place so that there may be
> other cases that can benefit from this change. Thanks for the new
> patch!
Pushed then.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
end of thread, other threads:[~2022-11-08 15:37 UTC | newest]
Thread overview: 21+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-09-19 14:28 BUG #17618: unnecessary filter column <> text even after adding index PG Bug reporting form <noreply@postgresql.org>
2022-09-19 15:24 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-09-19 15:45 ` Sindy Senorita <sindysenorita@gmail.com>
2022-09-20 17:12 ` Jeff Janes <jeff.janes@gmail.com>
2022-09-20 19:14 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-09-20 23:55 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-09-23 13:29 ` Richard Guo <guofenglinux@gmail.com>
2022-09-23 14:10 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-09-23 23:54 ` Richard Guo <guofenglinux@gmail.com>
2022-09-24 00:04 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-09-24 00:41 ` Richard Guo <guofenglinux@gmail.com>
2022-09-26 11:42 ` Richard Guo <guofenglinux@gmail.com>
2022-11-02 07:46 ` Richard Guo <guofenglinux@gmail.com>
2022-11-05 17:07 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-11-07 09:01 ` Richard Guo <guofenglinux@gmail.com>
2022-11-07 21:06 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-11-08 01:59 ` Richard Guo <guofenglinux@gmail.com>
2022-11-08 15:37 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-09-24 00:06 ` Richard Guo <guofenglinux@gmail.com>
2022-11-05 17:23 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-09-19 15:53 ` David G. Johnston <david.g.johnston@gmail.com>
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