From: Tom Lane <tgl@sss.pgh.pa.us>
To: Chao Li <li.evan.chao@gmail.com>
cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Fixing some ancient errors in hash join costing
In-reply-to: <F364A5CB-C518-4D28-8778-50F2F6BE9BDE@gmail.com>
References: <2380165.1766871097@sss.pgh.pa.us>
 <2551253.1766952956@sss.pgh.pa.us> <2712245.1766962600@sss.pgh.pa.us>
 <705AC80B-8E3A-4DA2-9FC2-A05C4D6C70FD@gmail.com>
 <2742257.1766978956@sss.pgh.pa.us>
 <F364A5CB-C518-4D28-8778-50F2F6BE9BDE@gmail.com>
Comments: In-reply-to Chao Li <li.evan.chao@gmail.com>
	message dated "Mon, 29 Dec 2025 14:19:05 +0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <2985327.1767031719.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Mon, 29 Dec 2025 13:08:39 -0500
Message-ID: <2985328.1767031719@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2985328.1767031719%40sss.pgh.pa.us>
Precedence: bulk

Chao Li <li.evan.chao@gmail.com> writes:
> On Dec 29, 2025, at 11:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Chao Li <li.evan.chao@gmail.com> writes:
>>> As flag 0 is passed to get_attstatsslot(), free_attstatsslot() is not =
needed.

>> True.  I wrote it like that so people wouldn't wonder if I'd forgotten
>> free_attstatsslot(), but if other call sites passing flags =3D=3D 0 don=
't
>> use it then it'd be better to be consistent.  (I didn't check that.)

> I searched over the source tree, looks like not a direct reference. The =
only usages of flag 0 is in eqjoinsel(), the code snippet is:

Yeah, that's not a direct precedent since there is a
free_attstatsslot() further down; nonetheless it is relying on the
call with flags=3D=3D0 to not do anything that'd need freeing, since
there's later calls that may overwrite the sslot structs.  But
get_attstatsslot's API spec is clear enough that you don't need
free_attstatsslot() in this usage, so I removed it.

> My thought was just that at the point of initialization, the nearby comm=
ent reads as if we=E2=80=99re about to fetch a value, whereas the assignme=
nt is really initializing with =E2=80=9Cunknown so far=E2=80=9D.  A short =
tweak like =E2=80=9CInitialize MCV frequency to unknown=E2=80=9D might hel=
p make that intent obvious locally, but I=E2=80=99m fine either way.

Fair enough, I modified it along those lines.

Pushed, thanks for reviewing.

			regards, tom lane