From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: Greg Stark <stark@mit.edu>, Andrey Lepikhov <a.lepikhov@postgrespro.ru>
Cc: Andres Freund <andres@anarazel.de>,
 Laurenz Albe <laurenz.albe@cybertec.at>,
 Hywel Carver <hywel@skillerwhale.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Removing unneeded self joins
Date: Tue, 17 May 2022 16:14:46 +0200
Message-ID: <2537176.Lt9SDvczpP@aivenronan>
Organization: aiven
In-Reply-To: <c0f41cba-659d-c6e1-7fb4-85e703dc2395@postgrespro.ru>
References: <64486b0b-0404-e39e-322d-0801154901f3@postgrespro.ru>
 <CAM-w4HMN5yax2bLWwknW5aB9Deg5A77JT1AXdVkdGVSVUpfhjQ@mail.gmail.com>
 <c0f41cba-659d-c6e1-7fb4-85e703dc2395@postgrespro.ru>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Archived-At: 
 <https://www.postgresql.org/message-id/2537176.Lt9SDvczpP%40aivenronan>
Precedence: bulk

Le vendredi 13 mai 2022, 07:07:47 CEST Andrey Lepikhov a =E9crit :
> New version of the feature.
> Here a minor bug with RowMarks is fixed. A degenerated case is fixed,
> when uniqueness of an inner deduced not from join quals, but from a
> baserestrictinfo clauses 'x=3Dconst', where x - unique field.
> Code, dedicated to solve second issue is controversial, so i attached
> delta.txt for quick observation.
> Maybe we should return to previous version of code, when we didn't split
> restriction list into join quals and base quals?

Hello,

I tried to find problematic cases, which would make the planning time grow=
=20
unacceptably, and couldn't devise it.

The worst case scenario I could think of was as follows:

 - a query with many different self joins
 - an abundance of unique indexes on combinations of this table columns to=
=20
consider
 - additional predicates on the where clause on columns.

The base table I used for this was a table with 40 integers. 39 unique inde=
xes=20
were defined on every combination of (c1, cX) with cX being columns c2 to c=
40.

I turned geqo off, set from_collapse_limit and join_collapse_limit to=20
unreasonably high values (30), and tried to run queries of the form:

SELECT * FROM test_table t1
JOIN test_table tX ON t1.c1 =3D tX.c1 AND t1.c[X+2] =3D tX.cX
=2E..
JOIN test_table tX ON t1.c1 =3D tX.c1 AND t1.c[X+2] =3D tX.cX.

So no self join can be eliminated in that case.
The performance was very similar with or without the GUC enabled. I tested =
the=20
same thing without the patch, since the test for uniqueness has been slight=
ly=20
altered and incurs a new allocation, but it doesn't seem to change.=20

One interesting side effect of this patch, is that removing any unneeded se=
lf=20
join cuts down the planification time very significantly, as we lower the n=
umber=20
of combinations to consider.=20

As for the code:=20

 - Comments on relation_has_unique_index_ext and relation_has_unique_index_=
for=20
should be rewritten, as relation_has_unique_index_for is now just a special=
=20
case of relation_has_unique_index_ext. By the way, the comment would probab=
ly=20
be better read as: "but if extra_clauses isn't NULL".
 - The whole thing about "extra_clauses", ie, baserestrictinfos which were=
=20
used to determine uniqueness, is not very clear. Most functions where the n=
ew=20
argument has been added have not seen an update in their comments, and the=
=20
name itself doesn't really convey the intented meaning: perhaps=20
required_non_join_clauses ?

The way this works should be explained a bit more thoroughly, for example i=
n=20
remove_self_joins_one_group the purpose of uclauses should be explained. Th=
e=20
fact that degenerate_case returns true when we don't have any additional ba=
se=20
restrict info is also confusing, as well as the degenerate_case name.

I'll update if I think of more interesting things to add.=20

=2D-=20
Ronan Dunklau