Minor refactor in catcache.c
Hi,
```
@@ -1010,7 +1010,7 @@ RehashCatCache(CatCache *cp)
dlist_foreach_modify(iter, &cp->cc_bucket[i])
{
CatCTup *ct = dlist_container(CatCTup, cache_elem, iter.cur);
- int hashIndex = HASH_INDEX(ct->hash_value, newnbuckets);
+ Index hashIndex = HASH_INDEX(ct->hash_value, newnbuckets); dlist_delete(iter.cur);
dlist_push_head(&newbucket[hashIndex], &ct->cache_elem);
@@ -1048,7 +1048,7 @@ RehashCatCacheLists(CatCache *cp)
dlist_foreach_modify(iter, &cp->cc_lbucket[i])
{
CatCList *cl = dlist_container(CatCList, cache_elem, iter.cur);
- int hashIndex = HASH_INDEX(cl->hash_value, newnbuckets);
+ Index hashIndex = HASH_INDEX(cl->hash_value, newnbuckets); dlist_delete(iter.cur);
dlist_push_head(&newbucket[hashIndex], &cl->cache_elem);
```
The 'hashIndex' should be 'Index' rather than 'int'.
```
@@ -2039,8 +2039,7 @@ SearchCatCacheList(CatCache *cache,
#ifndef CATCACHE_FORCE_RELEASE
ct->dead &&
#endif
- ct->refcount == 0 &&
- (ct->c_list == NULL || ct->c_list->refcount == 0))
+ ct->refcount == 0)
CatCacheRemoveCTup(cache, ct);
}
```Remove the dead code because we have a 'Assert(ct->c_list == NULL);'.
--
Regards,
ChangAo Chen
Attachments:
v1-0001-Minor-refactor-in-catcache.c.patchapplication/octet-stream; charset=utf-8; name=v1-0001-Minor-refactor-in-catcache.c.patchDownload
From 6f8ca1dc9d23426fb4a3614049ed7524bac67e0d Mon Sep 17 00:00:00 2001
From: ChangAo Chen <cca5507@qq.com>
Date: Tue, 30 Dec 2025 19:38:32 +0800
Subject: [PATCH v1] Minor refactor in catcache.c
---
src/backend/utils/cache/catcache.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 1d09c66ac95..8011be767aa 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -1010,7 +1010,7 @@ RehashCatCache(CatCache *cp)
dlist_foreach_modify(iter, &cp->cc_bucket[i])
{
CatCTup *ct = dlist_container(CatCTup, cache_elem, iter.cur);
- int hashIndex = HASH_INDEX(ct->hash_value, newnbuckets);
+ Index hashIndex = HASH_INDEX(ct->hash_value, newnbuckets);
dlist_delete(iter.cur);
dlist_push_head(&newbucket[hashIndex], &ct->cache_elem);
@@ -1048,7 +1048,7 @@ RehashCatCacheLists(CatCache *cp)
dlist_foreach_modify(iter, &cp->cc_lbucket[i])
{
CatCList *cl = dlist_container(CatCList, cache_elem, iter.cur);
- int hashIndex = HASH_INDEX(cl->hash_value, newnbuckets);
+ Index hashIndex = HASH_INDEX(cl->hash_value, newnbuckets);
dlist_delete(iter.cur);
dlist_push_head(&newbucket[hashIndex], &cl->cache_elem);
@@ -2039,8 +2039,7 @@ SearchCatCacheList(CatCache *cache,
#ifndef CATCACHE_FORCE_RELEASE
ct->dead &&
#endif
- ct->refcount == 0 &&
- (ct->c_list == NULL || ct->c_list->refcount == 0))
+ ct->refcount == 0)
CatCacheRemoveCTup(cache, ct);
}
--
2.34.1
On Tue, 30 Dec 2025 at 12:49, cca5507 <cca5507@qq.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)
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