Re: POC, WIP: OR-clause support for indexes

From: Andrei Lepikhov <a(dot)lepikhov(at)postgrespro(dot)ru>
To: jian he <jian(dot)universality(at)gmail(dot)com>
Cc: Alena Rybakina <a(dot)rybakina(at)postgrespro(dot)ru>, Alexander Korotkov <aekorotkov(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org, Peter Geoghegan <pg(at)bowt(dot)ie>, "Finnerty, Jim" <jfinnert(at)amazon(dot)com>, Marcos Pegoraro <marcos(at)f10(dot)com(dot)br>, teodor(at)sigaev(dot)ru, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, Peter Eisentraut <peter(at)eisentraut(dot)org>
Subject: Re: POC, WIP: OR-clause support for indexes
Date: 2024-02-12 05:52:13
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Thanks for the review!
It was the first version for discussion. Of course, refactoring and
polishing cycles will be needed, even so we can discuss the general idea

On 10/2/2024 12:00, jian he wrote:
> On Thu, Feb 8, 2024 at 1:34 PM Andrei Lepikhov
> 1235 | PredicatesData *pd;

> + if (!predicate_implied_by(index->indpred, list_make1(rinfo1), true))
> + elog(ERROR, "Logical mistake in OR <-> ANY transformation code");
> the error message seems not clear?
Yeah, have rewritten

> static List *
> build_paths_for_SAOP(PlannerInfo *root, RelOptInfo *rel, RestrictInfo *rinfo,
> List *other_clauses)
> I am not sure what's `other_clauses`, and `rinfo` refers to? adding
> some comments would be great.
> struct PredicatesData needs some comments, I think.
Added, not so much though
> +bool
> +saop_covered_by_predicates(ScalarArrayOpExpr *saop, List *predicate_lists)
> +{
> + ListCell *lc;
> + PredIterInfoData clause_info;
> + bool result = false;
> + bool isConstArray;
> +
> + Assert(IsA(saop, ScalarArrayOpExpr));
> is this Assert necessary?
Not sure. Moved it into another routine.
> For the function build_paths_for_SAOP, I think I understand the first
> part of the code.
> But I am not 100% sure of the second part of the `foreach(lc,
> predicate_lists)` code.
> more comments in `foreach(lc, predicate_lists)` would be helpful.
> do you need to add `PredicatesData` to src/tools/pgindent/typedefs.list?
> I also did some minor refactoring of generate_saop_pathlist.
Partially agree
> instead of let it go to `foreach (lc, entries)`,
> we can reject the Const array at `foreach(lc, expr->args)`
Yeah, I just think we can go further and transform two const arrays into
a new one if we have the same clause and operator. In that case, we
should allow it to pass through this cycle down to the classification stage.
> also `foreach(lc, expr->args)` do we need to reject cases like
> `contain_subplans((Node *) nconst_expr)`?
> maybe let the nconst_expr be a Var node would be far more easier.
It's contradictory. On the one hand, we simplify the comparison
procedure and can avoid expr jumbling at all. On the other hand - we
restrict the feature. IMO, it would be better to unite such clauses
complex_clause1 IN (..) OR complex_clause1 IN (..)
complex_clause1 IN (.., ..)
and don't do duplicated work computing complex clauses.
In the attachment - the next version of the second patch.

Andrei Lepikhov
Postgres Professional

Attachment Content-Type Size
0002-Teach-generate_bitmap_or_paths-to-build-BitmapOr-pat-20240212.patch text/plain 32.2 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Bharath Rupireddy 2024-02-12 06:46:41 Re: Add new error_action COPY ON_ERROR "log"
Previous Message Ashutosh Bapat 2024-02-12 04:42:21 Re: RFC: Logging plan of the running query