Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.100.0.2.22\))
Subject: Re: Planning performance problem (67626.278ms)
From: Manuel Weitzman <manuelweitzman@gmail.com>
In-Reply-To: <508897.1625005916@sss.pgh.pa.us>
Date: Wed, 30 Jun 2021 16:56:14 -0400
Cc: Ranier Vilela <ranier.vf@gmail.com>,
 David Rowley <dgrowleyml@gmail.com>,
 Jeremy Schneider <schnjere@amazon.com>,
 "pgsql-performance@lists.postgresql.org"
 <pgsql-performance@lists.postgresql.org>,
 Andres Freund <andres@anarazel.de>
Content-Transfer-Encoding: quoted-printable
Message-Id: <E4AB7AC2-596B-4B60-A5D4-D4BD48F9DE55@gmail.com>
References: <6d58f68e-ed1a-c48e-1440-691554f6e1e7@9livesdata.com>
 <CAFj8pRDDnOVNcU7dx4+oKH0x4-DsmL3XRWppQ8Cf5wwJ=G2fPg@mail.gmail.com>
 <20190408144210.GG10080@telsasoft.com>
 <b145b428-3351-ec86-d522-100a5bf8ded7@amazon.com>
 <20190408232644.r6xixkeulsmomwfj@alap3.anarazel.de>
 <3f5e5673-06aa-bf7c-cdb7-0e053c45ba26@amazon.com>
 <CAApHDvq9o1J45is6w-3Gv1UgtuDQ4eQP4UhFPQSnQmEH_EijvQ@mail.gmail.com>
 <767433.1619015825@sss.pgh.pa.us>
 <EFE95962-F088-45E2-BD13-AA5012829A69@gmail.com>
 <F06D0C00-DE75-4688-98B0-85A46E8F312C@gmail.com>
 <CAEudQApVF7TX-EJXwBz4TUhH6NzNB-wXqhs4wj+qSe6Vw3H5-Q@mail.gmail.com>
 <1588548.1624234651@sss.pgh.pa.us>
 <39C2266C-3143-4A7A-BCAF-E0C8394F9D24@gmail.com>
 <500142.1624995808@sss.pgh.pa.us>
 <F9C9FBE0-C535-49DA-B913-F11AC30F8F4E@gmail.com>
 <508897.1625005916@sss.pgh.pa.us>
To: Tom Lane <tgl@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/E4AB7AC2-596B-4B60-A5D4-D4BD48F9DE55%40gmail.com>
Precedence: bulk

> 1. create_join_clause doesn't trouble to look for commuted
> equivalents, which perhaps is penny-wise and pound-foolish.
> The cost of re-deriving selectivity estimates could be way
> more than the cost of checking this.

Agreed.

> 2. Although these look like they ought to be equivalent to the
> original clauses (modulo commutation, for some of them), they don't
> look that way to create_join_clause, because it's also checking
> for parent_ec equality.  Per the comment,
>=20
> * parent_ec is either equal to ec (if the clause is a =
potentially-redundant
> * join clause) or NULL (if not).  We have to treat this as part of the
> * match requirements --- it's possible that a clause comparing the =
same two
> * EMs is a join clause in one join path and a restriction clause in =
another.
>=20
> It might be worth digging into the git history to see why that
> became a thing and then considering whether there's a way around it.
> (I'm pretty sure that comment is mine, but I don't recall the details
> anymore.)

To me that sounds OK, I cannot prove that they're equivalent to the
original clauses so I think it is fine to assume they're not (not an
expert here, quite the opposite).

> Anyway, it's certainly not the case that we're making new
> RestrictInfos for every pair of rels.  It looks that way in this
> example because the join vars all belong to the same EC, but
> that typically wouldn't be the case in more complex queries.

Good to know, this wasn't clear to me.

> So we could look into whether this code can be improved to share
> RestrictInfos across more cases.  Another thought is that even
> if we need to keep original and derived clauses separate, maybe it'd
> be all right to copy previously-determined cached selectivity values
> from an original clause to an otherwise-identical derived clause
> (cf. commute_restrictinfo()).  I'm not sure though whether it's
> reliably the case that we'd have filled in selectivities for the
> original clauses before this code wants to clone them.

To be honest, even if that sounds like a good idea to dig on, I think
it wouldn't completely solve the problem with repeated calls to
get_actual_variable_range().

The example query I gave is doing a lot of simple auto-joins which
makes the thought process simpler, but I worry more about the more
"common" case in which there is more than 2 distinct tables involved
in the query

For example, instead of having "b1, b2, ..., bn" as aliases of "b" in
this query

>> explain (analyze, buffers)
>> select * from a
>> join b b1 on (b1.a =3D a.a)
>> join b b2 on (b2.a =3D a.a)
>> where b1.a in (1,100,10000,1000000,1000001);

it is also possible to reproduce the increasing cost in planning
buffers for each new join on a distinct table being added:

explain (analyze, buffers)
select * from a
join b on (b.a =3D a.a)
join c on (c.a =3D a.a)
-- ... (etc)
where c.a in (1,100,10000,1000000,1000001);

I can imagine that deconstruct_jointree() and
generate_join_implied_equalities() would generate multiple
RestrictInfos, in which many of them a constraint on a.a would be
involved (each involving a different table).

b.a =3D a.a
c.a =3D a.a
c.a =3D b.a
a.a =3D b.a
a.a =3D c.a
... (etc)

(if we wanted, we could also add a different WHERE clause on each of
the tables involved to make really sure all RestrictInfos are
different).

For each of these RestrictInfos there *could* be one cache miss on
cached_scansel() that *could* force the planner to compute
get_actual_variable_range() for the same variable (a.a) over and over,
as mergejoinscansel() always computes the selectivity for the
intervals that require actual extremal values. In practice this
re-computing of the variable range seems to happen a lot.

One way in which I see possible to share this kind of information (of
extremal values) across RestrictInfos is to store the known variable
ranges in PlannerInfo (or within a member of such struct), which seems
to be around everywhere it would be needed.


Best regards,
Manuel