Thread

Re: Minor refactor in catcache.c

cca5507 <cca5507@qq.com> — 2025-12-30T12:52:59Z

Hi,

> 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.

All other places in catcache.c the 'hashIndex' is 'Index', and the macro itself
also convert the result to 'Index':

#define HASH_INDEX(h, sz) ((Index) ((h) & ((sz) - 1)))

I think we should keep them consistent.

> 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.

After carefully reviewing the code, I can make sure that the 'ct->c_list == NULL'
is just always true. Your concerns also make sense to me. Let's see what others
think.

--
Regards,
ChangAo Chen