Thread

Re: Fixing some ancient errors in hash join costing

Tom Lane <tgl@sss.pgh.pa.us> — 2025-12-29T18:08:39Z

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 == 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==0 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 comment reads as if we’re about to fetch a value, whereas the assignment is really initializing with “unknown so far”.  A short tweak like “Initialize MCV frequency to unknown” might help make that intent obvious locally, but I’m fine either way.

Fair enough, I modified it along those lines.

Pushed, thanks for reviewing.

			regards, tom lane