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

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.
Agreed.
> 2. You can avoid the switch operator at the beginning of the function, 
> because you only need one option.
Agreed.
> 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 
otherwise:

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 
('{1,2}'::numeric[])))
    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,
                                                 scalar_type,
                                                 "IN");
      aexprs = lappend(aexprs, rexpr);
}

-- 
Regards,
Alena Rybakina
Postgres Professional