On 25.11.2023 19:13, Alexander Korotkov wrote:
On Sat, Nov 25, 2023 at 1:10 PM Alena Rybakina
<a.rybak...@postgrespro.ru> wrote:
On 25.11.2023 04:13, Alexander Korotkov wrote:
It seems to me there is a confusion. I didn't mean we need to move
conversion of OR-expressions to ANY into choose_bitmap_and() function
or anything like this. My idea was to avoid degradation of plans,
which I've seen in [1]. Current code for generation of bitmap paths
considers the possibility to split OR-expressions into distinct bitmap
index scans. But it doesn't consider this possibility for
ANY-expressions. So, my idea was to enhance our bitmap scan
generation to consider split values of ANY-expressions into distinct
bitmap index scans. So, in the example [1] and similar queries
conversion of OR-expressions to ANY wouldn't affect the generation of
bitmap paths.
Thanks for the explanation, yes, I did not understand the idea correctly at
first. I will try to implement something similar.
Alena, great, thank you. I'm looking forward to the updated patch.
I wrote the patch (any_to_or.diff.txt), although it is still under
development (not all regression tests have been successful so far), it
is already clear that for a query where a bad plan was chosen before, it
is now choosing a more optimal query plan.
postgres=# set enable_or_transformation =on;
SET
postgres=# explain select * from test where (x = 1 or x = 2) and y = 100;
QUERY PLAN
--------------------------------------------------------------------------------------------------------------
Bitmap Heap Scan on test (cost=8.60..12.62 rows=1 width=12)
Recheck Cond: (((y = '100'::double precision) AND (x = 1)) OR ((y =
'100'::double precision) AND (x = 2)))
-> BitmapOr (cost=8.60..8.60 rows=1 width=0)
-> Bitmap Index Scan on test_x_1_y (cost=0.00..4.30 rows=1
width=0)
Index Cond: (y = '100'::double precision)
-> Bitmap Index Scan on test_x_2_y (cost=0.00..4.30 rows=1
width=0)
Index Cond: (y = '100'::double precision)
(7 rows)
While I'm thinking how to combine it now.
BTW, while I was figuring out how create_index_paths works and creating
bitmapscan indexes, I think I found a bug with unallocated memory (fix
patch is bugfix.diff.txt). Without a fix here, it falls into the crust
at the stage of assigning a value to any of the variables, specifically,
skip_lower_stop and skip_nonnative_saop. I discovered it when I forced
to form a bitmapindex plan for ANY (any_to_or.diff.txt). I'm causing a
problem with my OR->ANY conversion patch.
--
Regards,
Alena Rybakina
Postgres Professional
diff --git a/src/backend/optimizer/path/indxpath.c
b/src/backend/optimizer/path/indxpath.c
index 03a5fbdc6dc..c242eec34d6 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -34,6 +34,7 @@
#include "optimizer/restrictinfo.h"
#include "utils/lsyscache.h"
#include "utils/selfuncs.h"
+#include "parser/parse_expr.h"
/* XXX see PartCollMatchesExprColl */
@@ -715,8 +716,8 @@ get_index_paths(PlannerInfo *root, RelOptInfo *rel,
List **bitindexpaths)
{
List *indexpaths;
- bool skip_nonnative_saop = false;
- bool skip_lower_saop = false;
+ bool skip_nonnative_saop;
+ bool skip_lower_saop;
ListCell *lc;
/*
@@ -737,7 +738,7 @@ get_index_paths(PlannerInfo *root, RelOptInfo *rel,
* that supports them, then try again including those clauses. This
will
* produce paths with more selectivity but no ordering.
*/
- if (skip_lower_saop)
+ if (skip_lower_saop || enable_or_transformation)
{
indexpaths = list_concat(indexpaths,
build_index_paths(root, rel,
@@ -778,7 +779,7 @@ get_index_paths(PlannerInfo *root, RelOptInfo *rel,
* natively, generate bitmap scan paths relying on executor-managed
* ScalarArrayOpExpr.
*/
- if (skip_nonnative_saop)
+ if (skip_nonnative_saop || enable_or_transformation)
{
indexpaths = build_index_paths(root, rel,
index, clauses,
@@ -908,7 +912,7 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
{
if (!index->amsearcharray)
{
- if (skip_nonnative_saop)
+ if (skip_nonnative_saop ||
enable_or_transformation)
{
/* Ignore because not supported
by index */
*skip_nonnative_saop = true;
@@ -919,7 +923,7 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
}
if (indexcol > 0)
{
- if (skip_lower_saop)
+ if (skip_lower_saop ||
enable_or_transformation)
{
/* Caller doesn't want to lose
index ordering */
*skip_lower_saop = true;
diff --git a/src/backend/optimizer/path/indxpath.c
b/src/backend/optimizer/path/indxpath.c
index 03a5fbdc6dc..21a6d8febf4 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -854,6 +854,9 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
bool index_only_scan;
int indexcol;
+ skip_lower_saop = (bool *) palloc(sizeof(bool));
+ skip_nonnative_saop = (bool *) palloc(sizeof(bool));
+
/*
* Check that index supports the desired scan type(s)
*/
diff --git a/src/backend/optimizer/path/allpaths.c
b/src/backend/optimizer/path/allpaths.c
index 67921a08262..3cddc0a4082 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -50,6 +50,8 @@
#include "port/pg_bitutils.h"
#include "rewrite/rewriteManip.h"
#include "utils/lsyscache.h"
+#include "parser/parse_expr.h"
+#include "utils/array.h"
/* Bitmask flags for pushdown_safety_info.unsafeFlags */
@@ -759,6 +761,179 @@ set_rel_consider_parallel(PlannerInfo *root, RelOptInfo
*rel,
rel->consider_parallel = true;
}
+static List*
+research_or_list(Expr *qual, List *or_list, RestrictInfo *sub_rinfo)
+{
+ List *elem_exprs = NIL;
+
+ /* Check: it is an expr of the form 'F(x) oper ConstExpr' */
+ if (!IsA(qual, ScalarArrayOpExpr))
+ {
+ /* Again, it's not the expr we can transform */
+ or_list = lappend(or_list, qual);
+ }
+ else
+ {
+ ScalarArrayOpExpr *saop = (ScalarArrayOpExpr *) qual;
+ Expr *rightop = (Expr *) lsecond(saop->args);
+ ListCell *lc1;
+
+ if (sub_rinfo && !op_mergejoinable(((OpExpr *)
sub_rinfo->clause)->opno, exprType(get_leftop(sub_rinfo->clause))))
+ {
+ /* And again, filter out non-equality operators
*/
+ or_list = lappend(or_list, (void *) qual);
+ }
+ else if (rightop && IsA(rightop, Const))
+ {
+ ArrayType *arrval;
+ int16 elemlen;
+ bool elembyval;
+ char elemalign;
+ Datum *elem_values;
+ bool *elem_nulls;
+ int num_elems,
+ i;
+ Const *arr = (Const *) rightop;
+
+ arrval = DatumGetArrayTypeP(arr->constvalue);
+ get_typlenbyvalalign(ARR_ELEMTYPE(arrval),
+ &elemlen,
&elembyval, &elemalign);
+ deconstruct_array(arrval,
+ ARR_ELEMTYPE(arrval),
+ elemlen, elembyval,
elemalign,
+ &elem_values,
&elem_nulls,
+ &num_elems);
+
+ for (i = 0; i < num_elems; i++)
+ {
+ Const *elem_expr;
+
+ /*
+ * A null array element must lead to a null
comparison result,
+ * since saop_op is known strict. We can ignore
it in the
+ * useOr case, but otherwise it implies
self-contradiction.
+ */
+ if (elem_nulls[i])
+ {
+ or_list = lappend(or_list, (void *)
qual);
+ elem_exprs = NIL;
+ break;
+ }
+
+ elem_expr = makeConst(ARR_ELEMTYPE(arrval), -1,
+
arr->constcollid, elemlen,
+
elem_values[i], false, elembyval);
+ elem_exprs = lappend(elem_exprs, elem_expr);
+ }
+ }
+ else if (rightop && IsA(rightop, ArrayExpr) && !((ArrayExpr *)
rightop)->multidims)
+ {
+ ArrayExpr *arrexpr = (ArrayExpr
*)get_rightop(rightop);
+ elem_exprs = arrexpr->elements;
+ }
+ else
+ or_list = lappend(or_list, qual);
+
+ if (elem_exprs)
+ foreach(lc1, elem_exprs)
+ {
+ Expr *elem_clause;
+
+ elem_clause = make_opclause(((ScalarArrayOpExpr*)
qual)->opno, BOOLOID, false,
+
(Expr *) linitial(((ScalarArrayOpExpr*)qual)->args), lfirst(lc1),
+
InvalidOid, ((ScalarArrayOpExpr*)qual)->inputcollid);
+ or_list = lappend(or_list, (void*) elem_clause);
+ }
+ }
+ return or_list;
+}
+
+static List *
+get_baserestrictinfo(PlannerInfo *root, List *baserestrictinfo)
+{
+ ListCell *lc;
+ List *modified_rinfo = NIL;
+ bool or_transformation = false;
+
+ if (!enable_or_transformation)
+ return NULL;
+
+ foreach(lc, baserestrictinfo)
+ {
+ RestrictInfo *rinfo_base = lfirst_node(RestrictInfo, lc);
+ RestrictInfo *rinfo;
+ List *or_list = NIL;
+
+ ListCell *lc_eargs,
+ *lc_rargs;
+
+ if (!IsA(rinfo_base->clause, BoolExpr) &&
!IsA(rinfo_base->clause, ScalarArrayOpExpr))
+ {
+ /* Add a clause without changes */
+ modified_rinfo = lappend(modified_rinfo, rinfo_base);
+ continue;
+ }
+
+ if (IsA(rinfo_base->clause, BoolExpr) &&
is_orclause(rinfo_base->clause))
+ forboth(lc_eargs, ((BoolExpr *) rinfo_base->clause)->args,
+ lc_rargs, ((BoolExpr *)
rinfo_base->orclause)->args)
+ {
+ Expr *orqual = (Expr *)
lfirst(lc_eargs);
+ RestrictInfo *sub_rinfo =
lfirst_node(RestrictInfo, lc_rargs);
+
+ if (!IsA(orqual, OpExpr) ||
+ !(bms_is_empty(sub_rinfo->left_relids) ^
+ bms_is_empty(sub_rinfo->right_relids)) ||
+ contain_volatile_functions((Node *) orqual))
+ {
+ /* Again, it's not the expr we can transform */
+ or_list = lappend(or_list, (void *) orqual);
+ continue;
+ }
+
+ or_list = research_or_list(orqual, or_list, sub_rinfo);
+ }
+ else if (IsA(rinfo_base->clause, ScalarArrayOpExpr))
+ {
+ Expr *orqual = rinfo_base->clause;
+
+ or_list = research_or_list(orqual, or_list, NULL);
+ }
+ else
+ {
+ or_list = lappend(or_list, (void*)rinfo_base->clause);
+ }
+
+
+ rinfo = make_restrictinfo(root,
+ list_length(or_list) > 1 ?
makeBoolExpr(OR_EXPR, or_list, -1):
+
(Expr *) linitial(or_list),
+ rinfo_base->is_pushed_down,
+ rinfo_base->has_clone,
+ rinfo_base->is_clone,
+ rinfo_base->pseudoconstant,
+ rinfo_base->security_level,
+ rinfo_base->required_relids,
+ rinfo_base->incompatible_relids,
+ rinfo_base->outer_relids);
+ rinfo->eval_cost=rinfo_base->eval_cost;
+ rinfo->norm_selec=rinfo_base->norm_selec;
+ rinfo->outer_selec=rinfo_base->outer_selec;
+ rinfo->left_bucketsize=rinfo_base->left_bucketsize;
+ rinfo->right_bucketsize=rinfo_base->right_bucketsize;
+ rinfo->left_mcvfreq=rinfo_base->left_mcvfreq;
+ rinfo->right_mcvfreq=rinfo_base->right_mcvfreq;
+ modified_rinfo = lappend(modified_rinfo, rinfo);
+ or_transformation = true;
+ }
+
+ if(or_transformation)
+ return modified_rinfo;
+
+ return baserestrictinfo;
+}
+
+
/*
* set_plain_rel_pathlist
* Build access paths for a plain relation (no subquery, no inheritance)
@@ -767,6 +942,7 @@ static void
set_plain_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte)
{
Relids required_outer;
+ List *baserestrict_base = copyObject(rel->baserestrictinfo);
/*
* We don't support pushing join clauses into the quals of a seqscan,
but
@@ -778,6 +954,8 @@ set_plain_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
RangeTblEntry *rte)
/* Consider sequential scan */
add_path(rel, create_seqscan_path(root, rel, required_outer, 0));
+ rel->baserestrictinfo = get_baserestrictinfo(root,
rel->baserestrictinfo);
+
/* If appropriate, consider parallel sequential scan */
if (rel->consider_parallel && required_outer == NULL)
create_plain_partial_paths(root, rel);
