Message-ID: <2a9604a5-520d-48ee-b3c8-5f3342b607d3@enterprisedb.com>
Date: Tue, 2 Apr 2024 20:22:55 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: using extended statistics to improve join estimates
To: Andy Fan <zhihuifan1213@163.com>, Justin Pryzby <pryzby@telsasoft.com>
Cc: Julien Rouhaud <rjuju123@gmail.com>, Andres Freund <andres@anarazel.de>,
 pgsql-hackers@postgresql.org, pgsql-hackers@lists.postgresql.org
References: <c8c0ff31-3a8a-7562-bbd3-78b2ec65f16c@enterprisedb.com>
 <2ed75657-e084-9539-c6de-597e5675014c@enterprisedb.com>
 <ff412e82-5427-3b72-cf32-4700156dc47c@enterprisedb.com>
 <20211122012315.GH17618@telsasoft.com>
 <0213d237-efd5-8e7e-fa29-3f36e6e26023@enterprisedb.com>
 <1ea3bf2b-57f9-e632-251a-e718623d7a72@enterprisedb.com>
 <20220104235550.dgr4rz6zkjx2rykz@alap3.anarazel.de>
 <20220119101809.wzrjvh4jsnoakgny@jrouhaud>
 <20220302173821.GG15744@telsasoft.com> <20220302173943.GA28810@telsasoft.com>
 <87cyr89nk5.fsf@163.com>
Content-Language: en-US
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <87cyr89nk5.fsf@163.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/2a9604a5-520d-48ee-b3c8-5f3342b607d3%40enterprisedb.com>
Precedence: bulk

On 4/2/24 10:23, Andy Fan wrote:
> 
>> On Wed, Mar 02, 2022 at 11:38:21AM -0600, Justin Pryzby wrote:
>>> Rebased over 269b532ae and muted compiler warnings.
> 
> Thank you Justin for the rebase!
> 
> Hello Tomas,
> 
> Thanks for the patch! Before I review the path at the code level, I want
> to explain my understanding about this patch first.
> 

If you want to work on this patch, that'd be cool. A review would be
great, but if you want to maybe take over and try moving it forward,
that'd be even better. I don't know when I'll have time to work on it
again, but I'd promise to help you with working on it.

> Before this patch, we already use MCV information for the eqjoinsel, it
> works as combine the MCV on the both sides to figure out the mcv_freq
> and then treat the rest equally, but this doesn't work for MCV in
> extended statistics, this patch fill this gap. Besides that, since
> extended statistics means more than 1 columns are involved, if 1+
> columns are Const based on RestrictInfo, we can use such information to
> filter the MCVs we are interesting, that's really cool. 
> 

Yes, I think that's an accurate description of what the patch does.

> I did some more testing, all of them are inner join so far, all of them
> works amazing and I am suprised this patch didn't draw enough
> attention. I will test more after I go though the code.
> 

I think it didn't go forward for a bunch of reasons:

1) I got distracted by something else requiring immediate attention, and
forgot about this patch.

2) I got stuck on some detail of the patch, unsure which of the possible
solutions to try first.

3) Uncertainty about how applicable the patch is in practice.

I suppose it was some combination of these reasons, not sure.


As for the "practicality" mentioned in (3), it's been a while since I
worked on the patch so I don't recall the details, but I think I've been
thinking mostly about "start join" queries, where a big "fact" table
joins to small dimensions. And in that case the fact table may have a
MCV, but the dimensions certainly don't have any (because the join
happens on a PK).

But maybe that's a wrong way to think about it - it was clearly useful
to consider the case with (per-attribute) MCVs on both sides as worth
special handling. So why not to do that for multi-column MCVs, right?

> At for the code level, I reviewed them in the top-down manner and almost
> 40% completed. Here are some findings just FYI. For efficiency purpose,
> I provide each feedback with a individual commit, after all I want to
> make sure my comment is practical and coding and testing is a good way
> to archive that. I tried to make each of them as small as possible so
> that you can reject or accept them convinently.
> 
> 0001 is your patch, I just rebase them against the current master. 0006
> is not much relevant with current patch, and I think it can be committed
> individually if you are OK with that.
> 
> Hope this kind of review is helpful.
> 

Cool! There's obviously no chance to get this into v18, and I have stuff
to do in this CF. But I'll take a look after that.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company