From: Chao Li <li.evan.chao@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Fixing some ancient errors in hash join costing
Date: Mon, 29 Dec 2025 08:39:38 +0800
Message-ID: <705AC80B-8E3A-4DA2-9FC2-A05C4D6C70FD@gmail.com> (raw)
In-Reply-To: <2712245.1766962600@sss.pgh.pa.us>
References: <2380165.1766871097@sss.pgh.pa.us>
<2551253.1766952956@sss.pgh.pa.us>
<2712245.1766962600@sss.pgh.pa.us>
> On Dec 29, 2025, at 06:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
>> I was amused to notice that the postgres_fdw.out change made in my
>> patch reverts one made in aa86129e1 (which also affected semijoin
>> costing). So we've had trouble before with that test case being
>> fundamentally unstable. I wonder if we shouldn't do something to try
>> to stabilize it? I see that the test immediately before this one
>> forces the matter by turning off enable_sort (which'd affect only
>> the local side not the remote). That's a hack all right but maybe
>> we should extend it to this test.
>
> Here's a v2 patchset that splits that out as a separate change for
> clarity's sake. I also spent a bit of effort on commit log messages,
> including researching the git history.
>
> regards, tom lane
>
Just a few small comments on v2:
1 - 0001
```
-SET enable_hashjoin TO off;
+-- These next tests require choosing between remote and local sort, which is
+-- a coin flip so long as cost_sort() gives the same results on both sides.
+-- To stabilize the expected plans, disable sorting locally.
SET enable_sort TO off;
-- subquery using stable function (can't be sent to remote)
+SET enable_hashjoin TO off; -- this one needs even more help to be stable
```
This is not a concern, just curious why switch the setting order of enable_hashjoin and enable_sort?
2 - 0002
```
+ else if (get_attstatsslot(&sslot, vardata.statsTuple,
+ STATISTIC_KIND_HISTOGRAM, InvalidOid,
+ 0))
+ {
+ /*
+ * If there are no recorded MCVs, but we do have a histogram, then
+ * assume that ANALYZE determined that the column is unique.
+ */
+ if (vardata.rel && vardata.rel->rows > 0)
+ *mcv_freq = 1.0 / vardata.rel->rows;
+ free_attstatsslot(&sslot);
+ }
```
As flag 0 is passed to get_attstatsslot(), free_attstatsslot() is not needed. The function comment of get_attstatsslot states about that:
```
* Passing flags=0 can be useful to quickly check if the requested slot type
* exists. In this case no arrays are extracted, so free_attstatsslot need
* not be called.
```
3 - In funciton estimate_hash_bucket_stats(), when mcv_freq is initialized:
```
/* Look up the frequency of the most common value, if available */
*mcv_freq = 0.0;
```
Maybe worth adding a short comment like “0.0 doesn’t mean zero frequency, instead 0.0 means no data or unknown frequency”, which might help code readers to quickly understand the logic.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: li.evan.chao@gmail.com, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: Fixing some ancient errors in hash join costing
In-Reply-To: <705AC80B-8E3A-4DA2-9FC2-A05C4D6C70FD@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox