-
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
Support MSSQL batch statements (Resolves #652) #666
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 2245625366
💛 - Coveralls |
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 for the PR!
For safety, make this optional. e.g. x-batch
.
See the postgres db driver's x-multi-statement
for an example
Thanks, @dhui. I'll look into those flags and add something by the next week |
@glebteterin heads up, I suspect that the changes merged in #714 will help you get passing sqlserver tests - they were broken for me prior to those changes, so be sure to pull them in! |
@jfhbrook-at-work can this be merged any time soon? I'm currently struggling with a migration that contains a couple of functions that can only be deployed to the database schema as a single batch statement. |
@tmeckel My team decided to make an internal fork and float this patch, rather than working to push it across the finish line. I can tell you, though, that this patch works fine for us - tests included. |
@jfhbrook-at-work thanks for the quick answer! So, for me it seems I either have to build a version on my own with this patch included or I wait for the team to implement this functionality aside this PR. If so, is there a time frame in which the functionality will be provided in an official build? |
I would say "when someone addresses the open issues in the PR". That's not gonna be me today - perhaps it will be you? |
@jfhbrook-at-work Okay, so the open issues are the ones @dhui mentioned in #666 (review)? |
That's my understanding, yes. |
@jfhbrook-at-work I just had a quick look how the stuff is implemented in the Postgres driver. Very straight forward. But I don't want to simply "hijack" the PR. @glebteterin will you have any time soon to implement the changes requested by @dhui |
The required changes are done, but I'm getting some flaky errors. @dhui any ideas what could it be? |
@glebteterin seems to me that the GitHub build agent wasn't able to start the database containers correctly. When I look over the build logs every single database test failed because the TCP connections couldn't be established. @jfhbrook-at-work can you restart the CI build? Perhaps this error was transient. |
@Fontinalis YEAH 🕺🏻 can't wait to get this merged 👍🏼 😄 |
@tmeckel Gonna check it one last time and merge it either this weekend or Monday. Had some last minute things coming up.. |
@Fontinalis will this PR merged soon? |
FYI, this PR will need to be updated after #758 is merged |
Hello, |
Hi, |
Hello, Could anyone please merge this PR ? |
@dhui would it be possible to merge this if someone addresses any remaining PR comments? We've been using it in a forked version for a year with no issues, but it would sure be nice if we could stop having to use/maintain our fork and just use the upstream : ) Cheers! |
Ping @dhui, is there anything I can do to help get this PR merged in? We've tested it for a year now and it works fine. Thanks!! |
Any update on this PR @Fontinalis? Not having this functionality is quite a big drawback! |
…to missing batch functionality (see golang-migrate/migrate#756 & golang-migrate/migrate#666). Also added 2 missing drop index statements in the golang-migrate down scripts.
The sql query gets split to batches with https://github.com/denisenkom/go-mssqldb/blob/master/batch/batch.go
Resolves #652 and #466