Fix bugs in lfc_cache_containsv #10672
Conversation
|
Can you split this into two PRs, please? Or even three, if the two fixed in lfc_cache_containsv` are easy to separate. Tests would be good. |
| /* Open cache file if not done yet */ | ||
| if (lfc_desc <= 0 && enabled) |
There was a problem hiding this comment.
We shouldn't be opening the LFC file if we've disabled the LFC.
-1 on this change
There was a problem hiding this comment.
Well, I think we really need to be able to reenable LFC.
I have added the following check to make sure that we open LFC file only if LFC was enabled or we are going to reenable it:
/* Open LFC file only if LFC was enabled or we are going to reenable it */
if ((newval > 0 || LFC_ENABLED()) && !lfc_ensure_opened())
return;
| tag.blockNum = (blkno + i) & ~(BLOCKS_PER_CHUNK - 1); | ||
| tag.blockNum = blkno & ~(BLOCKS_PER_CHUNK - 1); | ||
| hash = get_hash_value(lfc_hash, &tag); | ||
| chunk_offs = (blkno + i) & (BLOCKS_PER_CHUNK - 1); | ||
| chunk_offs = blkno & (BLOCKS_PER_CHUNK - 1); |
There was a problem hiding this comment.
Not really a bug getting fixed here, but alright.
There was a problem hiding this comment.
It is certainly not a bug, just some code simplification.
| while (true) | ||
| { | ||
| int this_chunk = Min(nblocks, BLOCKS_PER_CHUNK - chunk_offs); |
There was a problem hiding this comment.
Yep, that's a bug indeed, when crossing chunk boundaries. I'm not sure though why all those other things needed changing, IIUC those worked just fine.
7425 tests run: 7072 passed, 0 failed, 353 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
3e8faa8 at 2025-02-05T20:01:16.477Z :recycle: |
Problem
I have found one problem and two bugs in current LFC implementation.
neon.file_cache_size_limit=0then it can not be reenabled, becauselfc_change_limit_hookimmediately exit if!lfc_ensure_opened())and it is true in case ofneon.file_cache_size_limit=0.lfc_cache_containsv(and so affecting only pg17).Summary of changes
lfc_maybe_disabledcheck fromlfc_ensure_opened