On Tue, 30 Dec 2025 at 12:49, cca5507 <[email protected]> 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)
