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.

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 seqscan prefetch in pg17 #9865

Closed
wants to merge 5 commits into from
Closed

Fix seqscan prefetch in pg17 #9865

wants to merge 5 commits into from

Conversation

knizhnik
Copy link
Contributor

Problem

See #9860

Neon doesn't have seqscan detection of its own, so stop read_stream from trying to utilize that readahead, and instead make it issue readahead of its own.

Summary of changes

Add loop calling PrefetchBuffer for next effective_io_concurrency blocks.

@knizhnik knizhnik requested a review from a team as a code owner November 24, 2024 11:58
@knizhnik knizhnik requested a review from ololobus November 24, 2024 11:58
Copy link

github-actions bot commented Nov 24, 2024

6879 tests run: 6572 passed, 0 failed, 307 skipped (full report)


Flaky tests (2)

Postgres 17

Postgres 15

Code coverage* (full report)

  • functions: 31.0% (7969 of 25720 functions)
  • lines: 48.8% (63276 of 129701 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
5ecc4b2 at 2024-11-26T08:15:44.358Z :recycle:

@aswin-devil

This comment was marked as spam.

@hlinnaka
Copy link
Contributor

This addresses the same issue as #9860. I think PR #9860 is a better fix, read_stream.c should handle this in the new v17 model. I see your comment there that there's something wrong with the PR, so let's try to fix that.

@knizhnik
Copy link
Contributor Author

I am not sure that #9860 is better fix.
Unless I missed something, current AIO approach is to pin all prefetched buffers.
I think it will not work well in Neon when size of shared_buffers is small enough.
I wrote about it in my comment in Slack.

I will try to investigate more why prefetch distance in pg17 is 1, while max_ios is 100 (effective_io_ooncurency).
I suspect it is case caused by max pinned buffer limit. But doesn't matter where it comes from, art seems to be not the best idea to pin prefetched buffers.

@hlinnaka
Copy link
Contributor

I am not sure that #9860 is better fix. Unless I missed something, current AIO approach is to pin all prefetched buffers. I think it will not work well in Neon when size of shared_buffers is small enough. I wrote about it in my comment in Slack.

This is the way it's designed to work in upstream too. Our prefetching isn't that different from the upstream prefetching with posix_fadvise, nor the way async I/O will work in the future. So if there's a problem with pinning, we should address that.

We set shared_buffers to 16384. I think that leaves enough room for pinning buffers, even with pretty high effective_io_concurrency settings.

@MMeent
Copy link
Contributor

MMeent commented Dec 10, 2024

I think this is obsolete once #9860 merges (now only a matter of CI passing) so I'll close this one.

@MMeent MMeent closed this Dec 10, 2024
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.

4 participants