-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 Redshift migrations driver #128
Conversation
Avoid stepping on the 'redshift' driver for the time being. Driver registration is also modified to identify the clone as 'redshift2' rather than 'postgres'.
Redshift does not support advisory lock functions. The closest capability is in-transaction table locks, which aren't quite right here because the transaction scope is established within SetVersion, not higher up above the Lock-before/Unlock-after SetVersion. Local locking is left intact to satisfy expected "can't Lock twice before Unlocking" behavior asserted in shared tests.
This brings the 'redshift2' package in alignment with the existing 'redshift' package.
Redshift is based on Postgres version 8.0.2.
TRUNCATE commits the current transaction, which breaks the expection of the 'Commit()' that follows. See: golang-migrate#90 https://docs.aws.amazon.com/redshift/latest/dg/r_TRUNCATE.html
This addresses golang-migrate#90 . The exported Redshift object no longer exports an embedde 'Driver' however, so some more work is needed to make this backwards compatible.
Thanks for calling this out! Not embedding the postgres DB driver would be a backwards incompatible change but I don't know of a valid usecase for directly using an instance of the DB driver or using a DB driver's embeded field. But who knows what every consumer is doing?! I think it's fine to break backwards compatibility in this case since it's unlikely to affect anyone (famous last words) I don't think it's worth wrapping the new driver to preserve 100% backwards compatibility in this instance. We'll also announce this change in the release notes so in the unlikely event that this change breaks for someone, they'll know how to fix it. |
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.
I don't have access to a redshift cluster, so I'm relying on the rest of the community to test the changes.
@andrei-m Thanks for the PR! I'm going to wait a week to see if anyone is concerned about the backwards incompatible change before merging. I'll wait at least another week after that to cut a new release. |
Sounds great; thank you. |
This aims to address #90, specifically the high level plan outlined here: #90 (comment) .
The Redshift driver differs from the Postgres one in that:
In addition to unit tests, I ran a basic series of migrations against a real Redshift instance and verified that
transaction commit failed in line 0
no longer occurs.One question: Is it backwards-compatibility problem that the
redshift.Redshift
no longer has an exportedDriver
, which was previously an embedded interface? If so, any advice on how to approach that?