-
Notifications
You must be signed in to change notification settings - Fork 1.1k
MVP working transaction fix #8750
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
Conversation
* Automaticly migrate pg16 to pg17 * Formatting and cleanup --------- Co-authored-by: Joe Hanley <joehanley@google.com>
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.
LGTM, but I coauthored.
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.
Code looks good except one compatibility concern inline
| await tempOldDb.close(); | ||
| await oldDb.close(); | ||
|
|
||
| // 4. Nuke old data directory |
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.
Can we do this step last so that if something goes wrong during the import, the developer can still go back to an older CLI version?
Even better, what if we create a subdirectory in dataDir, say pg17 and only write to that subdirectory starting from this PR? In this way, the old and new CLI versions should not mess with each other. Ever time we change the on-disk format we can move to a different subdir name.
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.
That is a good call, implemented it:
When we migrate you, we'll copy your old dataDir over to dataDir/pg16. Then, we will use dataDir/pg17 going forward.
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.
Thanks, this looks great. I think we can make it even better by leaving existing data prefixless instead of pg16. In this way, downgrades are 100% safe out of the box without changing the dataDir settings.
Fixes transactions issues by updating to the latest version and removing the extended query patching because the upstream issue was fixed (electric-sql/pglite#294)
Updating pglite to 0.3.x also meant upgrading to Postgres 17 under the hood, which makes old data directories incompatible. To address this, we've added some automatic migration code based on https://pglite.dev/docs/upgrade
Fixes #8679 and #8658
Scenarios Tested
I grabbed a old data directory, ran
cat myData/PG_VERSIONto confirm it was PG16, and then ran this code against it and confirmed that it migrated and started up correctly. I then rerancat myData/PG_VERSIONto confirm it had been upgrade to PG17Code Samples