search

cmu-db / bustub

The BusTub Relational Database Management System (Educational)
https://15445.courses.cs.cmu.edu
MIT License
4.12k stars 1.81k forks source link

feat: Add support to PageGuard upgrade #581

Closed xzhseh closed 1 year ago

xzhseh commented 1 year ago

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 commented 1 year ago

Could I ask whether PageGuard involves the competing priority?

xzhseh commented 1 year ago

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!

xzhseh commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

skyzh commented 1 year ago

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 commented 1 year ago

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 🥰