public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andrei Lepikhov <[email protected]>
To: Alexander Korotkov <[email protected]>
To: Robert Haas <[email protected]>
Cc: Alena Rybakina <[email protected]>
Cc: jian he <[email protected]>
Cc: Nikolay Shaplov <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Peter Geoghegan <[email protected]>
Cc: Marcos Pegoraro <[email protected]>
Cc: [email protected]
Cc: Peter Eisentraut <[email protected]>
Cc: Ranier Vilela <[email protected]>
Subject: Re: POC, WIP: OR-clause support for indexes
Date: Wed, 16 Oct 2024 11:22:20 +0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAPpHfds3m=55cY1ea1TRzUAgD3pgwmvqm=exVMdOM4q+YT6kHg@mail.gmail.com>
References: <[email protected]>
<CAPpHfdtR3cBKCSj_7dd7EDF_pa-rUUQBx3-guK6kZZ=_hSeMfg@mail.gmail.com>
<[email protected]>
<CAPpHfdvF864n=Lzmjd2XBi9TwboZvrhRtLSt2hCP+JVUv6XKzg@mail.gmail.com>
<CACJufxHCJvC3X8nUK-jRvRru-ZEXp16EBPADOwTGaqmOYM1Raw@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAPpHfduBA9RXa1LQAz=wFK8cGx_AaHVK_vnNqCnG4yA7KwFf7g@mail.gmail.com>
<[email protected]>
<CAPpHfds4wGdWQ2xx1OUKZC4ggqS6Q=cBSck9Re1ajY0ESkwcmw@mail.gmail.com>
<CA+TgmoZ3tQ3Zwzo+9uXQL_tf0rDHQHKeZ4bGt1LDvL+_dqghUg@mail.gmail.com>
<CAPpHfdt8kowRDUkmOnO7_WJJQ1uk+O379JiZCk_9_Pt5AQ4+0w@mail.gmail.com>
<CAPpHfdu9QJ=Gbua3CUUH2KKG_8urakJTen4JD47PGh9wWP=QxQ@mail.gmail.com>
<CAPpHfds3m=55cY1ea1TRzUAgD3pgwmvqm=exVMdOM4q+YT6kHg@mail.gmail.com>
On 10/12/24 21:25, Alexander Korotkov wrote:
> I forgot to specify (COSTS OFF) for EXPLAINs in regression tests. Fixed in v42.
I've passed through the patch set.
Let me put aside the v42-0003 patch—it looks debatable, and I need time
to analyse the change in regression tests caused by this patch.
Comments look much better according to my current language level. Ideas
with fast exits also look profitable and are worth an additional
'matched' variable.
So, in general, it is ok. I think only one place with
inner_other_clauses can be improved. Maybe it will be enough to create
this list only once, outside 'foreach(j, groupedArgs)' cycle? Also, the
comment on the necessity of this operation was unclear to me. See the
attachment for my modest attempt at improving it.
--
regards, Andrei Lepikhov
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 5c8b64a5b1..0615c6d4de 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -1581,6 +1581,7 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
Path *bitmapqual;
ListCell *j;
List *groupedArgs;
+ List *inner_other_clauses = NIL;
/* Ignore RestrictInfos that aren't ORs */
if (!restriction_is_or_clause(rinfo))
@@ -1597,6 +1598,19 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
* because those RestrictInfos might match to the index as a whole.
*/
groupedArgs = group_similar_or_args(root, rel, rinfo);
+
+ if (groupedArgs != ((BoolExpr *) rinfo->orclause)->args)
+ /*
+ * Some parts of the rinfo were grouped. In this case we have a set
+ * of sub-rinfos that together is an exact duplicate of rinfo.
+ * In this case we need to remove the rinfo from other clauses.
+ * match_clauses_to_index detects duplicated iclause by comparing
+ * pointers to original rinfos that will be different. So, we must
+ * delete rinfo to avoid de-facto duplicated clauses in the index
+ * clauses list.
+ */
+ inner_other_clauses = list_delete(list_copy(all_clauses), rinfo);
+
foreach(j, groupedArgs)
{
Node *orarg = (Node *) lfirst(j);
@@ -1620,20 +1634,14 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
else if (restriction_is_or_clause(castNode(RestrictInfo, orarg)))
{
RestrictInfo *ri = castNode(RestrictInfo, orarg);
- List *inner_other_clauses;
/*
* Generate bitmap paths for the group of similar OR-clause
- * arguments. In this case we need to immediately remove the
- * rinfo from other clauses. This is because rinfo can be
- * transformed during index matching. So, we might be unable
- * to remove that later.
+ * arguments.
*/
- inner_other_clauses = list_delete(list_copy(all_clauses), rinfo);
indlist = make_bitmap_paths_for_or_group(root,
rel, ri,
inner_other_clauses);
- list_free(inner_other_clauses);
if (indlist == NIL)
{
@@ -1676,6 +1684,8 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
pathlist = lappend(pathlist, bitmapqual);
}
+ list_free(inner_other_clauses);
+
/*
* If we have a match for every arm, then turn them into a
* BitmapOrPath, and add to result list.
Attachments:
[text/plain] minor-fix.txt (2.4K, ../[email protected]/2-minor-fix.txt)
download | inline diff:
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 5c8b64a5b1..0615c6d4de 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -1581,6 +1581,7 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
Path *bitmapqual;
ListCell *j;
List *groupedArgs;
+ List *inner_other_clauses = NIL;
/* Ignore RestrictInfos that aren't ORs */
if (!restriction_is_or_clause(rinfo))
@@ -1597,6 +1598,19 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
* because those RestrictInfos might match to the index as a whole.
*/
groupedArgs = group_similar_or_args(root, rel, rinfo);
+
+ if (groupedArgs != ((BoolExpr *) rinfo->orclause)->args)
+ /*
+ * Some parts of the rinfo were grouped. In this case we have a set
+ * of sub-rinfos that together is an exact duplicate of rinfo.
+ * In this case we need to remove the rinfo from other clauses.
+ * match_clauses_to_index detects duplicated iclause by comparing
+ * pointers to original rinfos that will be different. So, we must
+ * delete rinfo to avoid de-facto duplicated clauses in the index
+ * clauses list.
+ */
+ inner_other_clauses = list_delete(list_copy(all_clauses), rinfo);
+
foreach(j, groupedArgs)
{
Node *orarg = (Node *) lfirst(j);
@@ -1620,20 +1634,14 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
else if (restriction_is_or_clause(castNode(RestrictInfo, orarg)))
{
RestrictInfo *ri = castNode(RestrictInfo, orarg);
- List *inner_other_clauses;
/*
* Generate bitmap paths for the group of similar OR-clause
- * arguments. In this case we need to immediately remove the
- * rinfo from other clauses. This is because rinfo can be
- * transformed during index matching. So, we might be unable
- * to remove that later.
+ * arguments.
*/
- inner_other_clauses = list_delete(list_copy(all_clauses), rinfo);
indlist = make_bitmap_paths_for_or_group(root,
rel, ri,
inner_other_clauses);
- list_free(inner_other_clauses);
if (indlist == NIL)
{
@@ -1676,6 +1684,8 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
pathlist = lappend(pathlist, bitmapqual);
}
+ list_free(inner_other_clauses);
+
/*
* If we have a match for every arm, then turn them into a
* BitmapOrPath, and add to result list.
view thread (78+ messages) latest in thread
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: POC, WIP: OR-clause support for indexes
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox