Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bugs in lfc_cache_containsv #10672

Closed
wants to merge 2 commits into from
Closed

Fix bugs in lfc_cache_containsv #10672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b36350b
Fix bugs in lfc_cache_containsv
Feb 5, 2025
3e8faa8
Call lfc_ensure_opened only when it is needed in lfc_change_limit_hook
Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 26 additions & 30 deletions pgxn/neon/file_cache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,10 +220,8 @@ lfc_maybe_disabled(void)
static bool
lfc_ensure_opened(void)
{
bool enabled = !lfc_maybe_disabled();

/* Open cache file if not done yet */
if (lfc_desc <= 0 && enabled)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be opening the LFC file if we've disabled the LFC.

-1 on this change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

if (lfc_desc <= 0)
{
lfc_desc = BasicOpenFile(lfc_path, O_RDWR);

Expand All @@ -233,7 +231,7 @@ lfc_ensure_opened(void)
return false;
}
}
return enabled;
return true;
}

static void
Expand Down Expand Up @@ -338,10 +336,11 @@ lfc_change_limit_hook(int newval, void *extra)
{
uint32 new_size = SIZE_MB_TO_CHUNKS(newval);

if (!is_normal_backend())
if (!lfc_ctl || !is_normal_backend())
return;

if (!lfc_ensure_opened())
/* Open LFC file only if LFC was enabled or we are going to reenable it */
if ((newval > 0 || LFC_ENABLED()) && !lfc_ensure_opened())
return;

LWLockAcquire(lfc_lock, LW_EXCLUSIVE);
Expand Down Expand Up @@ -509,47 +508,44 @@ lfc_cache_containsv(NRelFileInfo rinfo, ForkNumber forkNum, BlockNumber blkno,

CriticalAssert(BufTagGetRelNumber(&tag) != InvalidRelFileNumber);

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);
Comment on lines -512 to +513
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really a bug getting fixed here, but alright.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is certainly not a bug, just some code simplification.


LWLockAcquire(lfc_lock, LW_SHARED);

if (!LFC_ENABLED())
{
LWLockRelease(lfc_lock);
return 0;
}
while (true)
{
int this_chunk = Min(nblocks, BLOCKS_PER_CHUNK - chunk_offs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (LFC_ENABLED())
{
entry = hash_search_with_hash_value(lfc_hash, &tag, hash, HASH_FIND, NULL);
int this_chunk = Min(nblocks - i, BLOCKS_PER_CHUNK - chunk_offs);
entry = hash_search_with_hash_value(lfc_hash, &tag, hash, HASH_FIND, NULL);

if (entry != NULL)
if (entry != NULL)
{
for (; chunk_offs < BLOCKS_PER_CHUNK && i < nblocks; chunk_offs++, i++)
{
for (; chunk_offs < BLOCKS_PER_CHUNK && i < nblocks; chunk_offs++, i++)
if ((entry->bitmap[chunk_offs >> 5] &
((uint32)1 << (chunk_offs & 31))) != 0)
{
if ((entry->bitmap[chunk_offs >> 5] &
((uint32)1 << (chunk_offs & 31))) != 0)
{
BITMAP_SET(bitmap, i);
found++;
}
BITMAP_SET(bitmap, i);
found++;
}
}
else
{
i += this_chunk;
}
}
else
{
LWLockRelease(lfc_lock);
return found;
i += this_chunk;
}

/*
* Break out of the iteration before doing expensive stuff for
* a next iteration
*/
if (i + 1 >= nblocks)
if (i >= nblocks)
break;

/*
Expand All @@ -563,8 +559,8 @@ lfc_cache_containsv(NRelFileInfo rinfo, ForkNumber forkNum, BlockNumber blkno,

LWLockRelease(lfc_lock);

#if USE_ASSERT_CHECKING
do {
#ifdef USE_ASSERT_CHECKING
{
int count = 0;

for (int j = 0; j < nblocks; j++)
Expand All @@ -574,7 +570,7 @@ lfc_cache_containsv(NRelFileInfo rinfo, ForkNumber forkNum, BlockNumber blkno,
}

Assert(count == found);
} while (false);
}
#endif

return found;
Expand Down
9 changes: 6 additions & 3 deletions pgxn/neon/pagestore_smgr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ compact_prefetch_buffers(void)
target_slot->buftag = source_slot->buftag;
target_slot->shard_no = source_slot->shard_no;
target_slot->status = source_slot->status;
target_slot->flags = source_slot->flags;
target_slot->response = source_slot->response;
target_slot->reqid = source_slot->reqid;
target_slot->request_lsns = source_slot->request_lsns;
Expand Down Expand Up @@ -916,7 +917,7 @@ prefetch_register_bufferv(BufferTag tag, neon_request_lsns *frlsns,
{
uint64 min_ring_index;
PrefetchRequest hashkey;
#if USE_ASSERT_CHECKING
#ifdef USE_ASSERT_CHECKING
bool any_hits = false;
#endif
/* We will never read further ahead than our buffer can store. */
Expand Down Expand Up @@ -955,7 +956,7 @@ prefetch_register_bufferv(BufferTag tag, neon_request_lsns *frlsns,
else
lsns = NULL;

#if USE_ASSERT_CHECKING
#ifdef USE_ASSERT_CHECKING
any_hits = true;
#endif

Expand Down Expand Up @@ -1117,6 +1118,7 @@ prefetch_register_bufferv(BufferTag tag, neon_request_lsns *frlsns,
slot->buftag = hashkey.buftag;
slot->shard_no = get_shard_number(&tag);
slot->my_ring_index = ring_index;
slot->flags = 0;

min_ring_index = Min(min_ring_index, ring_index);

Expand Down Expand Up @@ -2845,12 +2847,13 @@ neon_prefetch(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
}

tag.blockNum = blocknum;

for (int i = 0; i < PG_IOV_MAX / 8; i++)
lfc_present[i] = ~(lfc_present[i]);

ring_index = prefetch_register_bufferv(tag, NULL, iterblocks,
lfc_present, true);

nblocks -= iterblocks;
blocknum += iterblocks;

Expand Down
Loading