Re: Minor refactor in catcache.c

On Tue, 30 Dec 2025 at 12:49, cca5507 <cca5507(at)qq(dot)com> wrote:
>
> Hi,
>
> [code]
> The 'hashIndex' should be 'Index' rather than 'int'.

I disagree: The Index type is used in planner/executor infrastructure
to refer to specific objects in arrays in plan trees; in normal
internal programming the int type is more than sufficient. In this
case, it's used to index into the hash buckets, and changing the type
of the variable to Index would only increase confusion for developers:
I can't think of any place where Index is used to refer to indexes
into non-planner arrays.

> @@ -2039,8 +2039,7 @@ SearchCatCacheList(CatCache *cache,
> [...]
> - ct->refcount == 0 &&
> - (ct->c_list == NULL || ct->c_list->refcount == 0))
> + ct->refcount == 0)
>
> Remove the dead code because we have a 'Assert(ct->c_list == NULL);'.

This code is in an exception catch block, so I'd be hesitant to remove
this check: it allows the code to more or less neatly handle the case
where the CatCTup refcount disagrees with its c_list membership(s)
when assertions are not enabled. Yes, it shouldn't happen, and we
Assert() that so that we can quickly identify the case in
assert-enabled builds, but were it to happen to a production system I
think this check would prevent further catcache state corruption,
rather than allowing it to spread.

Removing the check would increase our reliance on the continued
correctness of catcache's code, even in the face of exceptional
workflows, and I personally don't think the improved performance of 2
less conditions in this code are worth the risk.

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)