Skip to content

Conversation

@knizhnik
Copy link
Contributor

Problem

See #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.

@knizhnik knizhnik requested review from a team as code owners May 28, 2025 12:43
@knizhnik knizhnik requested review from MMeent, hlinnaka, myrrc and skyzh May 28, 2025 12:43
@github-actions
Copy link

If this PR added a GUC in the Postgres fork or neon extension,
please regenerate the Postgres settings in the cloud repo:

make NEON_WORKDIR=path/to/neon/checkout \
  -C goapp/internal/shareddomain/postgres generate

If you're an external contributor, a Neon employee will assist in
making sure this step is done.

@alexanderlaw
Copy link
Contributor

@knizhnik , could you please clarify why can't this usage of prefetch_register_bufferv() affect the release build?:

                        if (entry == NULL)
                        {
                                ring_index = prefetch_register_bufferv(hashkey.buftag, reqlsns, 1, NULL, false);
                                Assert(ring_index != UINT64_MAX);
                                slot = GetPrfSlot(ring_index);
                        }

Doesn't it matter which slot received here?

@github-actions
Copy link

github-actions bot commented May 28, 2025

8481 tests run: 7899 passed, 0 failed, 582 skipped (full report)


Flaky tests (1)

Postgres 17

Code coverage* (full report)

  • functions: 32.1% (9011 of 28033 functions)
  • lines: 48.4% (80033 of 165360 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
c83bd28 at 2025-06-10T09:57:37.817Z :recycle:

@knizhnik
Copy link
Contributor Author

@knizhnik , could you please clarify why can't this usage of prefetch_register_bufferv() affect the release build?:

                        if (entry == NULL)
                        {
                                ring_index = prefetch_register_bufferv(hashkey.buftag, reqlsns, 1, NULL, false);
                                Assert(ring_index != UINT64_MAX);
                                slot = GetPrfSlot(ring_index);
                        }

Doesn't it matter which slot received here?

as you can see only one page is requested here. The problem ca happen only when there are more two or more requests: during processing of second request first slot can be invalidated.
But if some errors happens during processing of first slot, we just repeat operation and return the right slot.

@alexanderlaw
Copy link
Contributor

I got it. Thank you for the explanation!

Copy link
Contributor

@MMeent MMeent left a comment

Choose a reason for hiding this comment

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

LGTM

@knizhnik knizhnik added this pull request to the merge queue Jun 10, 2025
Merged via the queue into main with commit 2194913 Jun 10, 2025
106 checks passed
@knizhnik knizhnik deleted the prefetch_register_bufferv_fix branch June 10, 2025 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assert(MyPState->ring_last <= min_ring_index ...) in prefetch_register_bufferv() fails

5 participants