Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: Fixing some ancient errors in hash join costing
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <2712245.1766962600@sss.pgh.pa.us>
Date: Mon, 29 Dec 2025 08:39:38 +0800
Cc: pgsql-hackers@lists.postgresql.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <705AC80B-8E3A-4DA2-9FC2-A05C4D6C70FD@gmail.com>
References: <2380165.1766871097@sss.pgh.pa.us>
 <2551253.1766952956@sss.pgh.pa.us> <2712245.1766962600@sss.pgh.pa.us>
To: Tom Lane <tgl@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/705AC80B-8E3A-4DA2-9FC2-A05C4D6C70FD%40gmail.com>
Precedence: bulk


> On Dec 29, 2025, at 06:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>=20
> 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.
>=20
> 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.
>=20
> regards, tom lane
>=20

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 =3D 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=3D0 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 =3D 0.0;
```

Maybe worth adding a short comment like =E2=80=9C0.0 doesn=E2=80=99t =
mean zero frequency, instead 0.0 means no data or unknown frequency=E2=80=9D=
, which might help code readers to quickly understand the logic.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/