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

feat: Add support to PageGuard upgrade #581

Closed
wants to merge 15 commits into from

Conversation

xzhseh
Copy link
Contributor

@xzhseh xzhseh commented Jun 22, 2023

This PR acts as a basic approach to issue #539, which adds two upgrade method, UpgradeRead() & UpgradeWrite to BasicPageGuard, also creates the corresponding GuardUpgradeTest, which is now disabled (Already been tested locally).

But if these two are left to future students, I can then remove the actual implementation part, and add guidelines to the comments🫣

@infdahai
Copy link

Could I ask whether PageGuard involves the competing priority?

@xzhseh
Copy link
Contributor Author

xzhseh commented Jun 27, 2023

Could I ask whether PageGuard involves the competing priority?

I rethought the upgrade logic, and found that we should drop the current BasicPageGuard first, and then fetch the corresponding ReadPageGuard or WritePageGuard from BPM. Otherwise it's not actually upgrade the protect logic for the inner Page.
Thanks for pointing this out!

Copy link
Member

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

In my definition of upgrade, it should be an atomic operation. For example, in read -> write upgrade, we will not drop the read lock. I think the same should apply to basic -> read upgrade.

I remembered there is a manual upgrade in table heap. Probably we can copy-paste that implementation.

Overall this sounds like a good practice for students and I would like to take this test case without implementation. I can remove UpgradeWrite/Read when I finish the refsol implementation and merge this.

auto *bpm = bpm_;
auto cur_pid = page_->GetPageId();
// First drop the current page guard
this->Drop();
Copy link
Member

Choose a reason for hiding this comment

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

In this implementation, there is possibility that the page will be evicted between L23 and L25?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I see, I ignore the inner condition when implementing the upgrade logic😭

@xzhseh
Copy link
Contributor Author

xzhseh commented Jul 1, 2023

In my definition of upgrade, it should be an atomic operation. For example, in read -> write upgrade, we will not drop the read lock. I think the same should apply to basic -> read upgrade.

I remembered there is a manual upgrade in table heap. Probably we can copy-paste that implementation.

Overall this sounds like a good practice for students and I would like to take this test case without implementation. I can remove UpgradeWrite/Read when I finish the refsol implementation and merge this.

I update the inner logic for UpgradeRead for BasicPageGuard(UpgradeWrite is kinda the same, so I just leave it empty), and the logic for ReadPageGuard::UpgradeWrite, but the semantic there I think should be further reviewed, detailed comments are included inside the function.

Also, I'd be glad to help write test cases, and I'll write more test cases in addition to basic sanity check, probably you can integrate some of the tests into Gradescope later, and I'll then remove them manually🥰

@xzhseh
Copy link
Contributor Author

xzhseh commented Jul 1, 2023

If the updated logic looks good, I'll just remove the implementation part, and leave it blank to the students, along with the tests.

@xzhseh
Copy link
Contributor Author

xzhseh commented Sep 29, 2023

Seems that PageGuard upgrade has been supported through reference solution.
The test cases in this PR could be of help, I can adjust them based on the current interface or add some more hidden tests if needed, cc @skyzh @yliang412.

@xzhseh
Copy link
Contributor Author

xzhseh commented Sep 29, 2023

Also, do we currently support upgrade from ReadPageGuard to WritePageGuard?

@skyzh
Copy link
Member

skyzh commented Sep 30, 2023

we do not plan to support read -> write b/c it's hard to make it atomic :(

Thanks for your contribution and I think we have incorporated what we need from this PR, and probably we can close it for now?

@xzhseh
Copy link
Contributor Author

xzhseh commented Sep 30, 2023

we do not plan to support read -> write b/c it's hard to make it atomic :(

Yea I've tried implementing this months before and also found it hard to maintain the atomicity without relying on some outer libraries 🤣

probably we can close it for now?

Sure! Thanks for reviewing 🥰

@xzhseh xzhseh closed this Sep 30, 2023
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.

3 participants