From: a.rybakina <a.rybakina@postgrespro.ru>
To: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Gregory Stark (as CFM) <stark.cfm@gmail.com>
Cc: Michał Kłeczek <michal@kleczek.org>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Removing unneeded self joins
Date: Fri, 13 Oct 2023 11:56:37 +0300
Message-ID: <29ddec15-a83f-4f24-b069-6e93c189b169@postgrespro.ru> (raw)
In-Reply-To: <9eb86ca9-da3c-417f-8ce1-29c0ef5a4f50@postgrespro.ru>
References: <64486b0b-0404-e39e-322d-0801154901f3@postgrespro.ru>
<0fd15c10-eaa5-51bd-b5c7-e635fe03a94d@postgrespro.ru>
<d9493d5a-b950-465e-f09b-62c92a23fba1@postgrespro.ru>
<20221206184643.zya44h5auv6f45zp@awork3.anarazel.de>
<36579124-f9d9-a51e-38b3-af3511921c7e@postgrespro.ru>
<88079CBD-B5BB-4C61-A68B-A8C67599E1C7@kleczek.org>
<CAM-w4HMZXW+hKc8C+WLQ+gdpEkoh9378a2UGKyZRz7mZVerG+Q@mail.gmail.com>
<c97574c4-13e6-9107-26ab-af2bd626fc85@postgrespro.ru>
<bcf2f3c5-fbdc-f85b-af3e-ebec616160f6@postgrespro.ru>
<05469e80-454b-41d7-d04f-421f42bbd4a9@postgrespro.ru>
<CAPpHfdt9G3EU+vXuqexhweEQoMF-acPUUFuB6hV2Kg0gCy7AjA@mail.gmail.com>
<ebe66358-428c-439f-85dd-da72c9b2b0c2@postgrespro.ru>
<CAPpHfdt-0kVV7O==aJEbjY2iGYBu+XBzTHEbPv_6sVNeC7fffQ@mail.gmail.com>
<48c11b23-9c23-440e-9a3c-020e05196449@postgrespro.ru>
<CAPpHfdsQkY0Gu7bQ9Ekdyzo_5Xxz74TYKbwmk1on1WAASpxXcQ@mail.gmail.com>
<9eb86ca9-da3c-417f-8ce1-29c0ef5a4f50@postgrespro.ru>
>> Also I've incorporated improvements from Alena Rybakina except one for
>> skipping SJ removal when no SJ quals is found. It's not yet clear for
>> me if this check fix some cases. But at least optimization got skipped
>> in some useful cases (as you can see in regression tests).
>
> Agree. I wouldn't say I like it too. But also, I suggest skipping some
> unnecessary assertions proposed in that patch:
> Assert(toKeep->relid != -1); - quite strange. Why -1? Why not all the
> negative numbers, at least?
> Assert(is_opclause(orinfo->clause)); - above we skip clauses with
> rinfo->mergeopfamilies == NIL. Each mergejoinable clause is already
> checked as is_opclause.
> All these changes (see in the attachment) are optional.
>
I don't mind about asserts, maybe I misunderstood something in the patch.
About skipping SJ removal when no SJ quals is found, I assume it is
about it:
split_selfjoin_quals(root, restrictlist, &selfjoinquals,
&otherjoinquals, inner->relid,
outer->relid);
+ if (list_length(selfjoinquals) == 0)
+ {
+ /*
+ * XXX:
+ * we would detect self-join without quals like 'x==x'
if we had
+ * an foreign key constraint on some of other quals
and this join
+ * haven't any columns from the outer in the target list.
+ * But it is still complex task.
+ */
+ continue;
+ }
as far as I remember, this is the place where it is checked that the SJ
list is empty and it is logical, in my opinion, that no transformations
should be performed if no elements are found for them.
As for the cases where SJ did not work, maybe this is just right if
there are no elements for processing these cases. I'll try to check or
come up with tests for them. If I'm wrong, write.
On 11.10.2023 06:51, Andrei Lepikhov wrote:
> On 11/10/2023 02:29, Alena Rybakina wrote:
>> I have reviewed your patch and I noticed a few things.
>
> Thanks for your efforts,
>
>> I have looked at the latest version of the code, I assume that the
>> error lies in the replace_varno_walker function, especially in the
>> place where we check the node by type Var, and does not form any
>> NullTest node.
>
> It's not a bug, it's an optimization we discussed with Alexander above.
>
>> Secondly, I added some code in some places to catch erroneous cases
>> and added a condition when we should not try to apply the
>> self-join-removal transformation due to the absence of an empty
>> self-join list after searching for it and in general if there are no
>> joins in the query. Besides, I added a query for testing and wrote
>> about it above. I have attached my diff file.
> Ok, I will look at this
>> In addition, I found a comment for myself that was not clear to me. I
>> would be glad if you could explain it to me.
>>
>> You mentioned superior outer join in the comment, unfortunately, I
>> didn't find anything about it in the PostgreSQL code, and this
>> explanation remained unclear to me. Could you explain in more detail
>> what you meant?
> I meant here that only clauses pushed by
> reconsider_outer_join_clauses() can be found in the joininfo list, and
> they are not relevant, as you can understand.
> Having written that, I realized that it was a false statement. ;) -
> joininfo can also contain results of previous SJE iterations, look:
>
> CREATE TABLE test (oid int PRIMARY KEY);
> CREATE UNIQUE INDEX ON test((oid*oid));
> explain
> SELECT count(*)
> FROM test c1, test c2, test c3
> WHERE c1.oid=c2.oid AND c1.oid*c2.oid=c3.oid*c3.oid;
> explain
> SELECT count(*)
> FROM test c1, test c2, test c3
> WHERE c1.oid=c3.oid AND c1.oid*c3.oid=c2.oid*c2.oid;
> explain
> SELECT count(*)
> FROM test c1, test c2, test c3
> WHERE c3.oid=c2.oid AND c3.oid*c2.oid=c1.oid*c1.oid;
Ok, I understood. Thank you for explanation.
--
Regards,
Alena Rybakina
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: pgsql-hackers@postgresql.org
Cc: a.rybakina@postgrespro.ru, a.lepikhov@postgrespro.ru, aekorotkov@gmail.com, stark.cfm@gmail.com, michal@kleczek.org, pgsql-hackers@lists.postgresql.org
Subject: Re: Removing unneeded self joins
In-Reply-To: <29ddec15-a83f-4f24-b069-6e93c189b169@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox