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: Mon, 04 Jul 2022 15:49:59 +0200
Message-ID: <2375492.jE0xQCEvom@aivenronan>
Organization: aiven
In-Reply-To: <e726cc25-de80-3818-66b2-1b893bb54602@postgrespro.ru>
References: <64486b0b-0404-e39e-322d-0801154901f3@postgrespro.ru>
 <13021914.uLZWGnKmhe@aivenronan>
 <e726cc25-de80-3818-66b2-1b893bb54602@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/2375492.jE0xQCEvom%40aivenronan>
Precedence: bulk

Le jeudi 30 juin 2022, 16:11:51 CEST Andrey Lepikhov a =E9crit :
> On 19/5/2022 16:47, Ronan Dunklau wrote:
> > I'll take a look at that one.
>=20
> New version of the patch, rebased on current master:
> 1. pgindent over the patch have passed.
> 2. number of changed files is reduced.
> 3. Some documentation and comments is added.

Hello Andrey,

Thanks for the updates.=20

The general approach seems sensible to me, so I'm going to focus on some de=
tails.=20

In a very recent thread [1], Tom Lane is proposing to add infrastructure to=
 make Var aware of their nullability by outer joins. I wonder if that would=
 help with avoiding the need for adding is not null clauses when the column=
 is known not null ?
If we have a precedent for adding a BitmapSet to the Var itself, maybe the =
whole discussion regarding keeping track of nullability can be extended to =
the original column nullability ?

Also, I saw it was mentioned earlier in the thread but how difficult would =
it be to process the transformed quals through the EquivalenceClass machine=
ry and the qual simplification ?=20
=46or example, if the target audience of this patch is ORM, or inlined view=
s, it wouldn't surprise me to see queries of this kind in the wild, which c=
ould be avoided altogether:

postgres=3D# explain (costs off) select * from sj s1 join sj s2 on s1.a =3D=
 s2.a where s1.b =3D 2 and s2.b =3D3;
                     QUERY PLAN                     =20
=2D----------------------------------------------------
 Seq Scan on sj s2
   Filter: ((a IS NOT NULL) AND (b =3D 3) AND (b =3D 2))
(2 lignes)


+       for (counter =3D 0; counter < list_length(*sources);)
+       {
+               ListCell   *cell =3D list_nth_cell(*sources, counter);
+               RestrictInfo *rinfo =3D castNode(RestrictInfo, lfirst(cell)=
);
+               int                     counter1;
+
=2E...
+                 ec->ec_members =3D list_delete_cell(ec->ec_members, cell);


Why don't you use foreach() and foreach_delete_current macros for iterating=
 and removing items in the lists, both in update_ec_members and update_ec_s=
ources ?


+                               if ((bms_is_member(k, info->syn_lefthand) ^
+                                        bms_is_member(r, info->syn_lefthan=
d)) ||
+                                       (bms_is_member(k, info->syn_rightha=
nd) ^
+                                        bms_is_member(r, info->syn_rightha=
nd)))

I think this is more compact and easier to follow than the previous version=
, but I'm not sure how common it is in postgres source code to use that kin=
d of construction ?

Some review about the comments:


I see you keep using the terms "the keeping relation" and "the removing rel=
ation" in reference to the relation that is kept and the one that is remove=
d.
Aside from the grammar (the kept relation or the removed relation), maybe i=
t would make it clearer to call them something else. In other parts of the =
code, you used "the remaining relation / the removed relation" which makes =
sense.

 /*
  * Remove the target relid from the planner's data structures, having
=2D * determined that there is no need to include it in the query.
+ * determined that there is no need to include it in the query. Or replace
+ * with another relid.
+ * To reusability, this routine can work in two modes: delete relid from a=
 plan
+ * or replace it. It is used in replace mode in a self-join removing proce=
ss.

This could be rephrased: ", optionally replacing it with another relid. The=
 latter is used by the self-join removing process."


[1] https://www.postgresql.org/message-id/flat/830269.1656693747%40sss.pgh.=
pa.us

=2D-=20
Ronan Dunklau