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
