From: Tom Lane <tgl@sss.pgh.pa.us>
To: Schoemans Maxime <maxime.schoemans@ulb.be>
cc: Damir Belyalov <dam.bel07@gmail.com>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
        SAKR Mahmoud <mahmoud.sakr@ulb.be>,
        Diogo Repas <diogo.repas@gmail.com>,
        LUO Zhicheng <zhicheng.luo@ulb.be>,
        Tomas Vondra <tomas.vondra@enterprisedb.com>,
        Andrey Lepikhov <a.lepikhov@postgrespro.ru>
Subject: Re: Implement missing join selectivity estimation for range types
In-reply-to: <4c443cf6-96af-0953-fd5b-bd2b5c0c6f00@ulb.be>
References: <CAB4o4asMq3k6HN9WfDsssQ5DDVfAziB4TpiFJ8RBJgZTVuwC7g@mail.gmail.com> <8afecd87-d1e5-241c-5e3e-75e1c62c279b@enterprisedb.com> <1bc5e1e4-380b-213d-6e6a-d5e8f7f0c7f5@enterprisedb.com> <CAB4o4aud47V_iRyWtA8+ZAmdXDjCF165R73AeCjx2RL0nzQzHA@mail.gmail.com> <84ffb566-8038-ab35-c841-7a5e5728a247@enterprisedb.com> <d46e1950-9228-3fe2-02b6-72ab7aaafb3a@enterprisedb.com> <CAB4o4asvPN=NT7KvS9zVQjZbdsiRW5t8aQctEkW7mxc4hbBxVQ@mail.gmail.com> <525ea957-88ff-e2c6-2975-474c849aa234@ulb.be> <ac128d6a-08a4-4848-e351-65cface6d6b5@ulb.be> <48a78946-f380-a279-e75f-711846e509f8@ulb.be> <CALH1Lgurg1y1DTeFOXOkpP=+X7saVGCh8gSjDLoCBOcFFWhz-A@mail.gmail.com> <4c443cf6-96af-0953-fd5b-bd2b5c0c6f00@ulb.be>
Comments: In-reply-to Schoemans Maxime <maxime.schoemans@ulb.be>
	message dated "Fri, 07 Jul 2023 15:40:43 -0000"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2516070.1699991181.1@sss.pgh.pa.us>
Date: Tue, 14 Nov 2023 14:46:21 -0500
Message-ID: <2516071.1699991181@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/2516071.1699991181%40sss.pgh.pa.us>
Precedence: bulk

Schoemans Maxime <maxime.schoemans@ulb.be> writes:
> You can find attached a new version of the patch that can be applied on 
> the current master branch of postgres.

I took a brief look through this very interesting work.  I concur
with Tomas that it feels a little odd that range join selectivity
would become smarter than scalar inequality join selectivity, and
that we really ought to prioritize applying these methods to that
case.  Still, that's a poor reason to not take the patch.

I also agree with the upthread criticism that having two identical
functions in different source files will be a maintenance nightmare.
Don't do it.  When and if there's a reason for the behavior to
diverge between the range and multirange cases, it'd likely be
better to handle that by passing in a flag to say what to do.

But my real unhappiness with the patch as-submitted is the test cases,
which require rowcount estimates to be reproduced exactly.  We know
very well that ANALYZE estimates are not perfectly stable and tend to
vary across platforms.  As a quick check I tried the patch within a
32-bit VM, and it passed, which surprised me a bit ... but it would
surprise me a lot if we got these same numbers on every machine in
the buildfarm.  We need a more forgiving test method.  Usually the
approach is to set up a test case where the improved accuracy of
the estimate changes the planner's choice of plan compared to what
you got before, since that will normally not be too prone to change
from variations of a percent or two in the estimates.  Another idea
could be something like

	SELECT (estimate/actual BETWEEN 0.9 AND 1.1) AS ok FROM ...

which just gives a true/false output instead of an exact number.

			regards, tom lane