migrate icon indicating copy to clipboard operation
migrate copied to clipboard

Support MSSQL batch statements (Resolves #652)

Open glebteterin opened this issue 4 years ago β€’ 24 comments

The sql query gets split to batches with https://github.com/denisenkom/go-mssqldb/blob/master/batch/batch.go Resolves #652 and #466

glebteterin avatar Dec 08 '21 18:12 glebteterin

Pull Request Test Coverage Report for Build 2245625366

  • 18 of 23 (78.26%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 57.861%

Changes Missing Coverage Covered Lines Changed/Added Lines %
database/sqlserver/sqlserver.go 18 23 78.26%
<!-- Total: 18 23
Totals Coverage Status
Change from base Build 1997075650: 0.05%
Covered Lines: 3754
Relevant Lines: 6488

πŸ’› - Coveralls

coveralls avatar Dec 08 '21 19:12 coveralls

@dhui thanks for taking a look at this. I can definitely look into those flags and at least confirm that the SQL Server tests pass, though like in #714 I probably won't tackle unrelated failures with other databases. Hopefully next week!

jfhbrook-at-work avatar Mar 17 '22 17:03 jfhbrook-at-work

Thanks, @dhui. I'll look into those flags and add something by the next week

glebteterin avatar Mar 17 '22 17:03 glebteterin

@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 avatar Mar 17 '22 19:03 jfhbrook-at-work

@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 avatar Apr 06 '22 16:04 tmeckel

@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 avatar Apr 06 '22 16:04 jfhbrook-at-work

@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?

tmeckel avatar Apr 06 '22 16:04 tmeckel

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 avatar Apr 06 '22 16:04 jfhbrook-at-work

@jfhbrook-at-work Okay, so the open issues are the ones @dhui mentioned in https://github.com/golang-migrate/migrate/pull/666#pullrequestreview-913412810?

tmeckel avatar Apr 06 '22 17:04 tmeckel

That's my understanding, yes.

jfhbrook-at-work avatar Apr 06 '22 17:04 jfhbrook-at-work

@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

tmeckel avatar Apr 06 '22 17:04 tmeckel

The required changes are done, but I'm getting some flaky errors. @dhui any ideas what could it be?

glebteterin avatar Apr 21 '22 02:04 glebteterin

@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.

tmeckel avatar Apr 21 '22 15:04 tmeckel

@Fontinalis YEAH πŸ•ΊπŸ» can't wait to get this merged πŸ‘πŸΌ πŸ˜„

tmeckel avatar Apr 29 '22 16:04 tmeckel

@tmeckel Gonna check it one last time and merge it either this weekend or Monday. Had some last minute things coming up..

Fontinalis avatar Apr 29 '22 16:04 Fontinalis

@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?

tmeckel avatar Jul 24 '22 19:07 tmeckel

FYI, this PR will need to be updated after #758 is merged

dhui avatar Jul 27 '22 06:07 dhui

Hello, Thanks for this PR I am also encountering this issue and have a need to support MSSQL batch statements in our migrate sql scripts. I see the https://github.com/golang-migrate/migrate/pull/758 is now merged. @dhui @Fontinalis any chances of this PR also getting merged anytime soon?

nishant-shah-social avatar Nov 11 '22 09:11 nishant-shah-social

Hi, Could you please finally merge it?

fildenisov avatar Dec 16 '22 05:12 fildenisov

Hello, Could anyone please merge this PR ?

meetsoni15 avatar Dec 16 '22 07:12 meetsoni15

@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!

ericsampson avatar Feb 28 '23 21:02 ericsampson

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!!

ericsampson avatar Jun 21 '23 22:06 ericsampson