A small problem when rehashing catalog cache
Hi,
When we search catalog cache, we move the searched tuple to the front of the list:
```
/*
* We found a match in the cache. Move it to the front of the list
* for its hashbucket, in order to speed subsequent searches. (The
* most frequently accessed elements in any hashbucket will tend to be
* near the front of the hashbucket's list.)
*/
dlist_move_head(bucket, &ct->cache_elem);
```
If I understand correctly, we reverse the list in RehashCatCache() and RehashCatCacheLists():
```
/* Move all entries from old hash table to new. */
for (i = 0; i < cp->cc_nbuckets; i++)
{
dlist_mutable_iter iter;
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);
dlist_delete(iter.cur);
dlist_push_head(&newbucket[hashIndex], &ct->cache_elem);
}
}
```
Maybe "dlist_push_head" -> "dlist_push_tail"? Thoughts?
--
Regards,
ChangAo Chen
Hi,
Although this does not affect correctness, I'd like to propose a patch to fix it.
--
Regards,
ChangAo Chen
Attachments:
v1-0001-Fix-unexpected-reversal-of-the-list-during-rehash.patchapplication/octet-stream; charset=utf-8; name=v1-0001-Fix-unexpected-reversal-of-the-list-during-rehash.patchDownload+2-3
On Wed, Dec 17, 2025 at 11:12:46AM +0800, cca5507 wrote:
Although this does not affect correctness, I'd like to propose a
patch to fix it.
That's an interesting point.
Indeed, the code bothers putting a fresh matching entry at the
beginning of a bucket, and the code does the opposite when moving
entries around, which is inconsistent to say the least. If we move
the entries at the tail instead as you are suggesting the "freshness"
would be preserved better. This deserves a comment, at least.
20cb18db4668 has added the RehashCatCache() part, with 473182c9523a
copying the same pattern for RehashCatCacheLists().
Thoughts or opinions from others?
--
Michael
This deserves a comment, at least.
How about adding this comment:
We keep recently searched entries at the front of the list, so we should call dlist_push_tail() here.
--
Regards,
ChangAo Chen
On Wed, Dec 17, 2025 at 10:35 AM Michael Paquier <michael@paquier.xyz> wrote:
Indeed, the code bothers putting a fresh matching entry at the
beginning of a bucket, and the code does the opposite when moving
entries around, which is inconsistent to say the least. If we move
the entries at the tail instead as you are suggesting the "freshness"
would be preserved better. This deserves a comment, at least.20cb18db4668 has added the RehashCatCache() part, with 473182c9523a
copying the same pattern for RehashCatCacheLists().Thoughts or opinions from others?
I suspect it doesn't make much difference in the grand scheme of
things, but the code has to do either one thing or the other, so +1 to
do the more sensible thing.
--
John Naylor
Amazon Web Services
On Mon, Jan 05, 2026 at 03:30:43PM +0700, John Naylor wrote:
I suspect it doesn't make much difference in the grand scheme of
things, but the code has to do either one thing or the other, so +1 to
do the more sensible thing.
Thanks for the input. I have applied the change as 68119480a763 on
HEAD, after adding a comment to document the expectation.
--
Michael