public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alexander Korotkov <[email protected]>
To: Alena Rybakina <[email protected]>
Cc: jian he <[email protected]>
Cc: Andrei Lepikhov <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Nikolay Shaplov <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Peter Geoghegan <[email protected]>
Cc: Marcos Pegoraro <[email protected]>
Cc: [email protected]
Cc: Peter Eisentraut <[email protected]>
Cc: Ranier Vilela <[email protected]>
Subject: Re: POC, WIP: OR-clause support for indexes
Date: Fri, 15 Nov 2024 15:27:43 +0200
Message-ID: <CAPpHfduzBgV3AecMU0jFqOSjK9iP86HiHEzj2Hv6hLqWu7JJFQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CACJufxHCJvC3X8nUK-jRvRru-ZEXp16EBPADOwTGaqmOYM1Raw@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAPpHfduBA9RXa1LQAz=wFK8cGx_AaHVK_vnNqCnG4yA7KwFf7g@mail.gmail.com>
<[email protected]>
<CAPpHfds4wGdWQ2xx1OUKZC4ggqS6Q=cBSck9Re1ajY0ESkwcmw@mail.gmail.com>
<CA+TgmoZ3tQ3Zwzo+9uXQL_tf0rDHQHKeZ4bGt1LDvL+_dqghUg@mail.gmail.com>
<CAPpHfdt8kowRDUkmOnO7_WJJQ1uk+O379JiZCk_9_Pt5AQ4+0w@mail.gmail.com>
<CAPpHfdu9QJ=Gbua3CUUH2KKG_8urakJTen4JD47PGh9wWP=QxQ@mail.gmail.com>
<CAPpHfds3m=55cY1ea1TRzUAgD3pgwmvqm=exVMdOM4q+YT6kHg@mail.gmail.com>
<[email protected]>
<CAPpHfdvxF1OZUoJr2bg8cmAnty-KyRkswPa3hCPSmR5gSx7-Yg@mail.gmail.com>
<CACJufxGS_MKqkfnw3BMhfi+=xuf2SAFvwf0Eq3e12XqAQaKdZg@mail.gmail.com>
<[email protected]>
Hi, Alena!
On Mon, Oct 28, 2024 at 6:55 PM Alena Rybakina
<[email protected]> wrote:
> I may be wrong, but the original idea was to double-check the result with the original expression.
>
> But I'm willing to agree with you. I think we should add transformed rinfo variable through add_predicate_to_index_quals function. I attached the diff file to the letter.
>
> diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
> index 3da7ea8ed57..c68ac7008e6 100644
> --- a/src/backend/optimizer/path/indxpath.c
> +++ b/src/backend/optimizer/path/indxpath.c
> @@ -3463,10 +3463,11 @@ match_orclause_to_indexcol(PlannerInfo *root,
> * rinfo in iclause->rinfo to detect duplicates and recheck the original
> * clause.
> */
> + RestrictInfo *rinfo_new = make_simple_restrictinfo(root,
> + &saopexpr->xpr);
> iclause = makeNode(IndexClause);
> - iclause->rinfo = rinfo;
> - iclause->indexquals = list_make1(make_simple_restrictinfo(root,
> - &saopexpr->xpr));
> + iclause->rinfo = rinfo_new;
> + iclause->indexquals = add_predicate_to_index_quals(index, list_make1(rinfo_new));
> iclause->lossy = false;
> iclause->indexcol = indexcol;
> iclause->indexcols = NIL;
As I stated in [1], I don't think we should pass transformed clause to
IndexClause.rinfo while comment explicitly says us to pass original
rinfo there.
> I figured out comments that you mentioned and found some addition explanation.
>
> As I understand it, this processing is related to ensuring that the selectivity of the index is assessed correctly and that there is no underestimation, which can lead to the selection of a partial index in the plan. See comment for the add_predicate_to_index_quals function:
>
> * ANDing the index predicate with the explicitly given indexquals produces
> * a more accurate idea of the index's selectivity. However, we need to be
> * careful not to insert redundant clauses, because clauselist_selectivity()
> * is easily fooled into computing a too-low selectivity estimate. Our
> * approach is to add only the predicate clause(s) that cannot be proven to
> * be implied by the given indexquals. This successfully handles cases such
> * as a qual "x = 42" used with a partial index "WHERE x >= 40 AND x < 50".
> * There are many other cases where we won't detect redundancy, leading to a
> * too-low selectivity estimate, which will bias the system in favor of using
> * partial indexes where possible. That is not necessarily bad though.
> *
> * Note that indexQuals contains RestrictInfo nodes while the indpred
> * does not, so the output list will be mixed. This is OK for both
> * predicate_implied_by() and clauselist_selectivity(), but might be
> * problematic if the result were passed to other things.
> */
>
> In those comments that you mentioned, it was written that this problem of expression redundancy is checked using the predicate_implied_by function, note that it is called there.
>
> * In some situations (particularly with OR'd index conditions) we may * have scan_clauses that are not equal to, but are logically implied by, * the index quals; so we also try a predicate_implied_by() check to see * if we can discard quals that way. (predicate_implied_by assumes its * first input contains only immutable functions, so we have to check * that.)
As the first line of header comment of add_predicate_to_index_quals()
says it adds partial index predicate to the quals list. I don't see
why should we use that in match_orclause_to_indexcol(), because this
function is only responsible to matching rinfo to particular index
column. Matching of partial index predicate is handled elsewhere.
Also check there is get_index_clause_from_support(), which is fetch
transformed clause from a support function. And it doesn't have to
fiddle with add_predicate_to_index_quals().
> I also figured out more information about loosy variable. First of all, I tried changing the value of the variable and did not notice any difference in regression tests. As I understood, our transformation is completely equivalent, so loosy should be true. But I don't think they are needed since our expressions are equivalent. I thought for a long time about an example where this could be a mistake and didn’t come up with any of them.
Yes, our transformation isn't lossy, thus IndexClause.lossy should be unset.
Links
1. https://www.postgresql.org/message-id/CAPpHfdvjtEWqjVcPd3-JQw8yCoppMXjK8kHnvinxBXGMZt-M_g%40mail.gma...
------
Regards,
Alexander Korotkov
Supabase
view thread (78+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: POC, WIP: OR-clause support for indexes
In-Reply-To: <CAPpHfduzBgV3AecMU0jFqOSjK9iP86HiHEzj2Hv6hLqWu7JJFQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox