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

From: Alena Rybakina <lena(dot)ribackina(at)yandex(dot)ru>
To: Andrey Lepikhov <a(dot)lepikhov(at)postgrespro(dot)ru>
Cc: Marcos Pegoraro <marcos(at)f10(dot)com(dot)br>, teodor(at)sigaev(dot)ru, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, Peter Geoghegan <pg(at)bowt(dot)ie>
Subject: Re: POC, WIP: OR-clause support for indexes
Date: 2023-07-07 08:20:20
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hi! Thank you for your detailed review, your changes have greatly helped
to improve this patch.

On 06.07.2023 13:20, Andrey Lepikhov wrote:
> On 6/7/2023 03:06, Alena Rybakina wrote:
>>> I corrected this constant in the patch.
> The patch don't apply cleanly: it contains some trailing spaces.
I fixed it.
> Also, quick glance into the code shows some weak points;
> 1. transformBoolExprOr should have input type BoolExpr.
> 2. You can avoid the switch operator at the beginning of the function,
> because you only need one option.
> 3. Stale comments: RestrictIinfos definitely not exists at this point.
Yes, unfortunately, I missed this from the previous version when I tried
to perform such a transformation at the index creation stage.
> 4. I don't know, you really need to copy the expr or not, but it is
> better to do as late, as possible.
Yes, I agree with you, copying "expr" is not necessary in this patch
> 5. You assume, that leftop is non-constant and rightop - constant. Why?
Agreed, It was too presumptuous on my part and I agree with your changes.
> 6.I doubt about equivalence operator. Someone can invent a custom '='
> operator with another semantics, than usual. May be better to check
> mergejoinability?
Yes, I agree with you, and I haven't thought about it before. But I
haven't found any functions to arrange this in PostgreSQL, but using
mergejoinability turns out to be more beautiful here.
> 7. I don't know how to confidently identify constant expressions at
> this level. So, I guess, You can only merge here expressions like
> "F(X)=Const", not an 'F(X)=ConstExpression'.
I see, you can find solution for this case, thank you for this, and I
think it's reliable enough.

On 07.07.2023 05:43, Andrey Lepikhov wrote:
> On 6/7/2023 03:06, Alena Rybakina wrote:
>>> The test was performed on the same benchmark database generated by 2
>>> billion values.
>>> I corrected this constant in the patch.
> In attempt to resolve some issues had mentioned in my previous letter
> I used op_mergejoinable to detect mergejoinability of a clause.
> Constant side of the expression is detected by call of
> eval_const_expressions() and check each side on the Const type of node.
> See 'diff to diff' in attachment.

I notices you remove condition for checking equal operation.

strcmp(strVal(linitial((arg)->name)), "=") == 0

Firstly, it is noticed me not correct, but a simple example convinced me

postgres=# explain analyze select x from a where x=1 or x>5 or x<3 or x=2;
                                               QUERY PLAN
 Seq Scan on a  (cost=0.00..2291.00 rows=97899 width=4) (actual
time=0.038..104.168 rows=99000 loops=1)
   Filter: ((x > '5'::numeric) OR (x < '3'::numeric) OR (x = ANY
   Rows Removed by Filter: 1000
 Planning Time: 9.938 ms
 Execution Time: 113.457 ms
(5 rows)

It surprises me that such check I can write such similar way:

eval_const_expressions(NULL, orqual).

Yes, I see we can remove this code:

bare_orarg = transformExprRecurse(pstate, (Node *)arg);
bare_orarg = coerce_to_boolean(pstate, bare_orarg, "OR");

because we will provide similar manipulation in this:

foreach(l, gentry->consts)
      Node       *rexpr = (Node *) lfirst(l);

      rexpr = coerce_to_common_type(pstate, rexpr,
     aexprs = lappend(aexprs, rexpr);

Alena Rybakina
Postgres Professional

Attachment Content-Type Size
0001-Replace-OR-clause-to-ANY-expressions.patch text/x-patch 9.2 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Masahiko Sawada 2023-07-07 08:21:04 Re: Add hint message for check_log_destination()
Previous Message Damir Belyalov 2023-07-07 08:08:48 Re: Implement missing join selectivity estimation for range types