Enable read only DB tests on windows #4200
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ray6080
approved these changes
Sep 5, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4200 +/- ##
==========================================
+ Coverage 86.65% 86.75% +0.10%
==========================================
Files 1327 1327
Lines 51365 51581 +216
Branches 6895 6948 +53
==========================================
+ Hits 44511 44751 +240
+ Misses 6682 6658 -24
Partials 172 172 ☔ View full report in Codecov by Sentry. |
Benchmark ResultMaster commit hash:
|
benjaminwinger
force-pushed
the
windows-read-only
branch
from
c17874d to
18b92c0
Compare
Benchmark ResultMaster commit hash:
|
benjaminwinger
commented
Sep 6, 2024
| Exception, | ||
| match=r"RuntimeError: IO exception: Could not set lock on file", | ||
| ): | ||
| open_database_on_subprocess(db_path, build_dir) |
There was a problem hiding this comment.
I did some tests with the shell, and as far as I can tell on windows you can open the same database in read-write mode, operate on the files, and the files get clobbered by whichever writes last. The locks don't seem to be working properly. It works as expected if you open one in read-only mode though.
The C++ test that does something similar is being skipped, presumably because in addition to windows not supporting this initially, it also wasn't written to work with msvc (among other things it uses fork to create the process, which doesn't exist on windows).
There was a problem hiding this comment.
Yeah, the lock is not supported on windows yet. #921
There was a problem hiding this comment.
I guess not. I'd seen locking code from #2089, but it doesn't appear to work.
I have a suspicion that the issue is that it locks a region of the file, not the entire file, so locking write access to a zero-length region (as our code currently does) doesn't actually do anything. But maybe it would work if we lock even a single byte instead.
benjaminwinger
force-pushed
the
windows-read-only
branch
2 times, most recently
from
559b2cd to
ba4822e
Compare
Benchmark ResultMaster commit hash:
|
benjaminwinger
force-pushed
the
windows-read-only
branch
2 times, most recently
from
6e8a160 to
d19b19d
Compare
Benchmark ResultMaster commit hash:
|
benjaminwinger
force-pushed
the
windows-read-only
branch
from
d19b19d to
eb12321
Compare
Benchmark ResultMaster commit hash:
|
benjaminwinger
force-pushed
the
windows-read-only
branch
from
eb12321 to
105e629
Compare
Benchmark ResultMaster commit hash:
|
benjaminwinger
force-pushed
the
windows-read-only
branch
3 times, most recently
from
5beaf88 to
ce4de37
Compare
Benchmark ResultMaster commit hash:
|
benjaminwinger
force-pushed
the
windows-read-only
branch
from
ce4de37 to
472ceb4
Compare
Benchmark ResultMaster commit hash:
|
benjaminwinger
force-pushed
the
windows-read-only
branch
6 times, most recently
from
7d8270b to
d12f370
Compare
benjaminwinger
force-pushed
the
windows-read-only
branch
from
d12f370 to
3fb4af2
Compare
Benchmark ResultMaster commit hash:
|
ted-wq-x
pushed a commit
to ted-wq-x/kuzu
that referenced
this pull request
Nov 14, 2024
(cherry picked from commit d491c4f)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support for this was implemented a while ago (I don't recall exactly when), but evidently these tests were missed.