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

Check CLI version and fix race condition in join-order scanning #1490

Merged
merged 2 commits into from
Sep 1, 2022

Conversation

dbartol
Copy link
Contributor

@dbartol dbartol commented Aug 31, 2022

Two fixes, both serious regressions introduced by the "bad join order detection" feature.

See individual commit messages for details.

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.
@dbartol dbartol added bug Something isn't working Complexity: Low A good task for newcomers to learn, or experienced team members to complete quickly. labels Aug 31, 2022
@dbartol dbartol requested a review from a team as a code owner August 31, 2022 22:21
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

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)?

@dbartol
Copy link
Contributor Author

dbartol commented Sep 1, 2022

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.

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

This makes sense. Thanks for the fix!

@dbartol dbartol merged commit ac74b96 into github:main Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Complexity: Low A good task for newcomers to learn, or experienced team members to complete quickly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants