-
Notifications
You must be signed in to change notification settings - Fork 53
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
fix parallel test error on update #246
fix parallel test error on update #246
Conversation
I think this should solve the issue now. I didn't modify the workflow, I wasn't sure if we wanted the check to fail if the update tests fail. Otherwise I think this is ready to merge |
Looks good! |
The branch is up to date with main, I just pushed changes to make the workflow fail when update tests fail. If you have logs of the issue I can take a look, it sounds like I may still have some bugs to work through EDIT: ah I'm seeing it now, maybe it's related to the |
Actually I was having that issue when the |
I can recreate this locally, I'm not totally sure what the issue is, it seems like there are maybe the update scripts aren't compatible with older versions of postgres. The crash seems to come from violating an assertion in the scan. It also seems like the concurrent update may not have been the reindex but something else in the script which is probably good news. Relevant section of logs below
|
It seems for some reason the indices are not reindexed after the upgrade. The |
…oading of common into one time only section
@var77 I think it should work across all the versions now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
left_cursor REFCURSOR; | ||
left_row RECORD; | ||
|
||
right_cursor REFCURSOR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is quite some repetition between this file and the common.sql in non-parallel tests. Can we somehow avoid copying and have the file in one of the two locations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense. No reason we can't use one file
closes #226
This is a workspace to try solutions to #226. Apologies if this is spamming test result notifications
EDIT: it looks like the reindexing operations from the version update scripts are getting rolled into the regression tests. I'm pretty sure this is because the update is done in the test runner which is run with each individual test in the group. The original error comes from multiple parallel tests (runners) trying to reindex the same tables at once. Additionally for some reason if only one test is run the output from the reindexing process is rolled into the regression test. I don't know exactly how pg_regress handles outputs from the test runner and the test itself, but it feels plausible to me that it merges them.