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.
