Check CLI version and fix race condition in join-order scanning#1490
Merged
dbartol merged 2 commits intogithub:mainfrom Sep 1, 2022
dbartol:dbartol/log-version/work
Merged
Check CLI version and fix race condition in join-order scanning#1490dbartol merged 2 commits intogithub:mainfrom dbartol:dbartol/log-version/work
dbartol merged 2 commits intogithub:mainfrom
dbartol:dbartol/log-version/work
Conversation
added 2 commits
August 31, 2022 18:08
The original code that logged the human-readable log summary generated the log asynchronously, which was a reasonable choice. When I added support for viewing and scanning logs, I didn't notice that the summary was being generated asynchronously, and wrote my code assuming that the summary was already on disk when I opened it to find where each relation's log started. The effect was that, depending on timing, the evaluation sometimes failed with an error popup complaining about not being able to open the log summary file. The fix is to _generate_ the log summary synchronously, but continue to _log_ it asynchronously.
adityasharad
approved these changes
Aug 31, 2022
Contributor
adityasharad
left a comment
There was a problem hiding this comment.
Looks reasonable. Do we have a test that exercising the reading of the log summary (granted that may not have failed consistently with this bug)?
Contributor
Author
|
Any of our tests that run a query would hit the racy code path. I never hit it in my initial development, but I hit it pretty regularly while debugging the version check issue. I think it had something to do with where my breakpoints were set. |
aeisenberg
approved these changes
Sep 1, 2022
Contributor
aeisenberg
left a comment
There was a problem hiding this comment.
This makes sense. Thanks for the fix!
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Two fixes, both serious regressions introduced by the "bad join order detection" feature.
See individual commit messages for details.