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

Perform inmem_smgr cleaup after processing each record #154

Merged
merged 4 commits into from
Apr 27, 2022
Merged

Conversation

knizhnik
Copy link

No description provided.

@hlinnaka
Copy link
Contributor

So, I realized that this isn't quite right either. Imagine that num_temp_buffers == 4, and you have a WAL replay function that does this:

target_buf = XLogReadBufferForRedo(<Target page>);
<apply changes on the target page>
MarkBufferDirty(target_buf);
UnlockReleaseBuffer(target_buf);
other_buf1 = XLogReadBufferForRedo(<Target page>);
other_buf2 = XLogReadBufferForRedo(<Target page>);
other_buf3 = XLogReadBufferForRedo(<Target page>);
other_buf4 = XLogReadBufferForRedo(<Target page>);

This will cause the target page to be evicted from the buffer cache, and stored in the "inmem_smgr". After replaying the record, we will call smgrinit(), which will throw away the target page and the changes that were applied to it.

So we need to prevent the target page from being thrown away. I see two options:

  1. hack the page eviction code in localbuf.c to never evict the redo target buffer, or
  2. hack inmem_smgr.c to not throw away the target page

@hlinnaka
Copy link
Contributor

I tried to change zenith_wal_redo.c so that it keeps the target page pinned, from BeginRedoForBlock() until the GetPage() call. That would give a tiny speedup too, by avoiding the hash lookup in GetPage() and PushPage(). But that didn't work, because some WAL redo functions take the "cleanup lock" on the page, which requires that there are no other pins on the page.

@knizhnik
Copy link
Author

This will cause the target page to be evicted from the buffer cache, a
I do not think that it is possible.
Please notice that XLogReadBufferForRedo ignores all records except target records and only for it returns BLK_NEEDS_REDO. Actually it can not get content of this page: smgr is transient and only one page image is received from page server. So redo can only construct new pages. But it is only done when XLogReadBufferForRedo returns BLK_NEEDS_REDO. So I do not think that target page can be somehow evicted from buffers and pinning is actually neded.
But we can add some asserts here...

@hlinnaka
Copy link
Contributor

hlinnaka commented Apr 26, 2022

This will cause the target page to be evicted from the buffer cache, a

I do not think that it is possible.
Please notice that XLogReadBufferForRedo ignores all records except target records and only for it returns BLK_NEEDS_REDO. Actually it can not get content of this page: smgr is transient and only one page image is received from page server. So redo can only construct new pages. But it is only done when XLogReadBufferForRedo returns BLK_NEEDS_REDO. So I do not think that target page can be somehow evicted from buffers and pinning is actually neded.
But we can add some asserts here...

Well, in RBM_ZERO_AND_LOCK mode it is possible, at least.

@knizhnik
Copy link
Author

Well, in RBM_ZERO_AND_LOCK mode it is possible, at least.

I see.
Unfortunately I do not know better solution than just patch bufmgr to prohibit eviction of such page.
See my proposed implementation. Do not like it but...

@@ -1201,6 +1204,13 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,

Assert(BUF_STATE_GET_REFCOUNT(buf_state) == 0);

if (buf->buf_id == wal_redo_buffer)
Copy link
Contributor

Choose a reason for hiding this comment

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

We use the local buffer manager in the WAL redo process, so this needs to be in localbuf.c

Copy link
Author

Choose a reason for hiding this comment

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

Shame on me:(
Yet another attempt.

@hlinnaka
Copy link
Contributor

Feel free to take over neondatabase/neon#1573 to test this, btw.

@knizhnik knizhnik requested a review from hlinnaka April 27, 2022 13:23
@knizhnik
Copy link
Author

So the tests are passed. What's then?

Copy link
Contributor

@hlinnaka hlinnaka left a comment

Choose a reason for hiding this comment

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

Let's squash and merge this

@knizhnik knizhnik merged commit a13fe64 into main Apr 27, 2022
@knizhnik knizhnik deleted the inmem_cleanup branch April 27, 2022 14:36
MMeent pushed a commit that referenced this pull request Jul 7, 2022
* Perform inmem_smgr cleaup after processing each record

* Prevent eviction of wal redo target page

* Prevent eviction of wal redo target page frmo temp buffers
MMeent pushed a commit that referenced this pull request Aug 18, 2022
* Perform inmem_smgr cleaup after processing each record

* Prevent eviction of wal redo target page

* Prevent eviction of wal redo target page frmo temp buffers
lubennikovaav pushed a commit that referenced this pull request Nov 21, 2022
* Perform inmem_smgr cleaup after processing each record

* Prevent eviction of wal redo target page

* Prevent eviction of wal redo target page frmo temp buffers
MMeent pushed a commit that referenced this pull request Feb 10, 2023
* Perform inmem_smgr cleaup after processing each record

* Prevent eviction of wal redo target page

* Prevent eviction of wal redo target page frmo temp buffers
MMeent pushed a commit that referenced this pull request Feb 10, 2023
* Perform inmem_smgr cleaup after processing each record

* Prevent eviction of wal redo target page

* Prevent eviction of wal redo target page frmo temp buffers
MMeent pushed a commit that referenced this pull request May 11, 2023
* Perform inmem_smgr cleaup after processing each record

* Prevent eviction of wal redo target page

* Prevent eviction of wal redo target page frmo temp buffers
tristan957 pushed a commit that referenced this pull request Aug 10, 2023
* Perform inmem_smgr cleaup after processing each record

* Prevent eviction of wal redo target page

* Prevent eviction of wal redo target page frmo temp buffers
tristan957 pushed a commit that referenced this pull request Nov 8, 2023
* Perform inmem_smgr cleaup after processing each record

* Prevent eviction of wal redo target page

* Prevent eviction of wal redo target page frmo temp buffers
tristan957 pushed a commit that referenced this pull request Nov 8, 2023
* Perform inmem_smgr cleaup after processing each record

* Prevent eviction of wal redo target page

* Prevent eviction of wal redo target page frmo temp buffers
tristan957 pushed a commit that referenced this pull request Feb 5, 2024
* Perform inmem_smgr cleaup after processing each record

* Prevent eviction of wal redo target page

* Prevent eviction of wal redo target page frmo temp buffers
tristan957 pushed a commit that referenced this pull request Feb 5, 2024
* Perform inmem_smgr cleaup after processing each record

* Prevent eviction of wal redo target page

* Prevent eviction of wal redo target page frmo temp buffers
tristan957 pushed a commit that referenced this pull request Feb 6, 2024
* Perform inmem_smgr cleaup after processing each record

* Prevent eviction of wal redo target page

* Prevent eviction of wal redo target page frmo temp buffers
tristan957 pushed a commit that referenced this pull request May 10, 2024
* Perform inmem_smgr cleaup after processing each record

* Prevent eviction of wal redo target page

* Prevent eviction of wal redo target page frmo temp buffers
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.

2 participants