From 21949137ed0db4429f79d8532103ded958f87634 Mon Sep 17 00:00:00 2001 From: Konstantin Knizhnik Date: Tue, 10 Jun 2025 13:09:46 +0300 Subject: [PATCH] Return last ring index instead of min_ring_index in prefetch_register_bufferv (#12039) ## Problem See https://github.com/neondatabase/neon/issues/12018 Now `prefetch_register_bufferv` calculates min_ring_index of all vector requests. But because of pump prefetch state or connection failure, previous slots can be already proceeded and reused. ## Summary of changes Instead of returning minimal index, this function should return last slot index. Actually result of this function is used only in two places. A first place just fort checking (and this check is redundant because the same check is done in `prefetch_register_bufferv` itself. And in the second place where index of filled slot is actually used, there is just one request. Sp fortunately this bug can cause only assert failure in debug build. --------- Co-authored-by: Konstantin Knizhnik --- pgxn/neon/communicator.c | 47 ++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/pgxn/neon/communicator.c b/pgxn/neon/communicator.c index 2655a45bcc..7c84be7d15 100644 --- a/pgxn/neon/communicator.c +++ b/pgxn/neon/communicator.c @@ -1092,13 +1092,15 @@ communicator_prefetch_register_bufferv(BufferTag tag, neon_request_lsns *frlsns, MyPState->ring_last <= ring_index); } -/* internal version. Returns the ring index */ +/* Internal version. Returns the ring index of the last block (result of this function is used only +* when nblocks==1) +*/ static uint64 prefetch_register_bufferv(BufferTag tag, neon_request_lsns *frlsns, BlockNumber nblocks, const bits8 *mask, bool is_prefetch) { - uint64 min_ring_index; + uint64 last_ring_index; PrefetchRequest hashkey; #ifdef USE_ASSERT_CHECKING bool any_hits = false; @@ -1122,13 +1124,12 @@ Retry: MyPState->ring_unused - MyPState->ring_receive; MyNeonCounters->getpage_prefetches_buffered = MyPState->n_responses_buffered; + last_ring_index = UINT64_MAX; - min_ring_index = UINT64_MAX; for (int i = 0; i < nblocks; i++) { PrefetchRequest *slot = NULL; PrfHashEntry *entry = NULL; - uint64 ring_index; neon_request_lsns *lsns; if (PointerIsValid(mask) && BITMAP_ISSET(mask, i)) @@ -1152,12 +1153,12 @@ Retry: if (entry != NULL) { slot = entry->slot; - ring_index = slot->my_ring_index; - Assert(slot == GetPrfSlot(ring_index)); + last_ring_index = slot->my_ring_index; + Assert(slot == GetPrfSlot(last_ring_index)); Assert(slot->status != PRFS_UNUSED); - Assert(MyPState->ring_last <= ring_index && - ring_index < MyPState->ring_unused); + Assert(MyPState->ring_last <= last_ring_index && + last_ring_index < MyPState->ring_unused); Assert(BufferTagsEqual(&slot->buftag, &hashkey.buftag)); /* @@ -1169,9 +1170,9 @@ Retry: if (!neon_prefetch_response_usable(lsns, slot)) { /* Wait for the old request to finish and discard it */ - if (!prefetch_wait_for(ring_index)) + if (!prefetch_wait_for(last_ring_index)) goto Retry; - prefetch_set_unused(ring_index); + prefetch_set_unused(last_ring_index); entry = NULL; slot = NULL; pgBufferUsage.prefetch.expired += 1; @@ -1188,13 +1189,12 @@ Retry: */ if (slot->status == PRFS_TAG_REMAINS) { - prefetch_set_unused(ring_index); + prefetch_set_unused(last_ring_index); entry = NULL; slot = NULL; } else { - min_ring_index = Min(min_ring_index, ring_index); /* The buffered request is good enough, return that index */ if (is_prefetch) pgBufferUsage.prefetch.duplicates++; @@ -1283,12 +1283,12 @@ Retry: * The next buffer pointed to by `ring_unused` is now definitely empty, so * we can insert the new request to it. */ - ring_index = MyPState->ring_unused; + last_ring_index = MyPState->ring_unused; - Assert(MyPState->ring_last <= ring_index && - ring_index <= MyPState->ring_unused); + Assert(MyPState->ring_last <= last_ring_index && + last_ring_index <= MyPState->ring_unused); - slot = GetPrfSlotNoCheck(ring_index); + slot = GetPrfSlotNoCheck(last_ring_index); Assert(slot->status == PRFS_UNUSED); @@ -1298,11 +1298,9 @@ Retry: */ slot->buftag = hashkey.buftag; slot->shard_no = get_shard_number(&tag); - slot->my_ring_index = ring_index; + slot->my_ring_index = last_ring_index; slot->flags = 0; - min_ring_index = Min(min_ring_index, ring_index); - if (is_prefetch) MyNeonCounters->getpage_prefetch_requests_total++; else @@ -1315,11 +1313,12 @@ Retry: MyPState->ring_unused - MyPState->ring_receive; Assert(any_hits); + Assert(last_ring_index != UINT64_MAX); - Assert(GetPrfSlot(min_ring_index)->status == PRFS_REQUESTED || - GetPrfSlot(min_ring_index)->status == PRFS_RECEIVED); - Assert(MyPState->ring_last <= min_ring_index && - min_ring_index < MyPState->ring_unused); + Assert(GetPrfSlot(last_ring_index)->status == PRFS_REQUESTED || + GetPrfSlot(last_ring_index)->status == PRFS_RECEIVED); + Assert(MyPState->ring_last <= last_ring_index && + last_ring_index < MyPState->ring_unused); if (flush_every_n_requests > 0 && MyPState->ring_unused - MyPState->ring_flush >= flush_every_n_requests) @@ -1335,7 +1334,7 @@ Retry: MyPState->ring_flush = MyPState->ring_unused; } - return min_ring_index; + return last_ring_index; } static bool