Allow running Golang based post migration steps #1253
Conversation
|
|
||
| // If there is a post execution function for | ||
| // this migration, run it now. | ||
| cb, ok := m.opts.postStepCallbacks[migr.Version] |
There was a problem hiding this comment.
IIUC if the post step callback fails, then the version remains dirty. This could end up being a problem requiring down migration from the user. I wonder if it'd be better to just run the migration, the callback and a final SetVersion in a transaction?
There was a problem hiding this comment.
Yeah, I agree. But the thing is: The version is also dirty if the SQL based migration fails, as we also error out then.
And there doesn't seem to be the concept of DB transactions in the migration tool, my guess is because not every supported database backend can do transactions...
So not really sure what to do differently here.
There was a problem hiding this comment.
IIUC if the post step callback fails, then the version remains dirty.
This appears to be aa quirk related to the way the library works:
Lines 62 to 65 in 604248c
In the context of our desired usage we typically catch situations like this via unit tests of the migration itself.
There was a problem hiding this comment.
After reading a bit more of the codebase, I think it's possible to run the callback function in the same db transaction as the migration:
migrate/database/sqlite/sqlite.go
Lines 201 to 204 in 604248c
With the way the interfaces work, if we modify those (using something other than a func opt), then we'd need to update every single driver in the codebase.
Perhaps the slimmest change would be to add the functional opt to the Run method in the main Driver interface?
This is a preparatory commit that adds the ability to add new options to any of the constructor functions, without breaking backward compatibility. An actual option is going to be added in the next commit, this just introduces the mechanism.
guggero
force-pushed
the
post-migrate-exec
branch
from
0e91936 to
08353de
Compare
There was a problem hiding this comment.
Looks like a great addition to me! The post-migration step callback executes while the driver lock—held during the migration—is still in place. And if the callback fails, it results in the same failed state as a migration failure, so no change in behavior there.
| err := cb(migr, m.databaseDrv) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I think you should wrap err in the return statement with a contextual message using fmt.Errorf for better clarity.
return fmt.Errorf("failed to execute post migration callback: %w", err)
This is an alternative approach to #932.
Often more complex data migrations cannot be fully expressed in SQL alone.
This PR introduces the ability to run a Golang based callback directly after a SQL based migration step was executed (and before the version is updated from dirty to clean in the migration table).
The new feature can be seen in action here: lightninglabs/taproot-assets@1b9a8cc