Skip to content
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

Non-transactional SQL callbacks failing after upgrade to 4.2.0 #1707

Closed
aebaugh opened this issue Jul 12, 2017 · 6 comments
Closed

Non-transactional SQL callbacks failing after upgrade to 4.2.0 #1707

aebaugh opened this issue Jul 12, 2017 · 6 comments

Comments

@aebaugh
Copy link

@aebaugh aebaugh commented Jul 12, 2017

Flyway 4.2.0
Command-line client
Microsoft SQL Server 13.0 (AWS RDS Enterprise SQL Server)
Flyway 4.1.2 and 4.2.0, both using jtds 1.3.1
repros running from OSX, Linux (assume platform independent)

We have previously worked around the inability to create a database on SQL Server within a migration by putting the DDL within a beforeMigrate.sql callback.
Example beforeMigrate.sql:

-- Create overall database
IF NOT EXISTS(SELECT * FROM sys.databases WHERE name = '${DBNAME}')
   BEGIN
    PRINT 'Creating ${DBNAME} Database'
    CREATE DATABASE ${DBNAME}
   END
ELSE
    PRINT '${DBNAME} Database Exists'
GO

With 4.1.2 this continues to work as expected.
After upgrading to 4.2.0:

Executing SQL callback: beforeMigrate
WARNING: DB: Creating <dbname> Database (SQL State: null - Error Code: 0)
ERROR: 
Migration beforeMigrate.sql failed
----------------------------------
SQL State  : S1000
Error Code : 226
Message    : CREATE DATABASE statement not allowed within multi-statement transaction.
Location   :  <filelocation>

Although I believe some SQL Server DDL statements support transactions, creating a database with other statements does not appear to be.

Initially thought this might be related to #181 involving the flyway.group configuration introduced in 4.2.0, but after stepping through with a debugger it does not appear to be as essentially the same codepaths are exercised. It also doesn't look to be related to autocommit or other connection properties.

I believe it is instead related to #1588 changing SQL Server to single connection mode. Hypothesis is that the statements executed on the connection (for metadata, schema, etc) prior to the callback lead to the error mentioned for create database DDL.

Sample command line with above beforeMigrate.sql (and no additional migration .sql files):
flyway -X -url=jdbc:jtds:sqlserver://<rds_endpoint>:1433/tempdb -schemas=flyway -user=<user> -password=<pwd> -locations=filesystem:db_init_script -placeholders.DBNAME=<dbname> migrate

Also tested this first executing SET IMPLICIT_TRANSACTIONS OFF with no difference, so do not think it's related or a workaround.

I am not set up to recompile SQLServerDbSupport.java and change useSingleConnection() to return false, which should prove/disprove.

Would it make sense to have this be a configurable property? e.g. flyway.useSingleConnection=false
I can imagine there may be other scenarios where this may not be desired, based on the scripts being executed.

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jul 12, 2017

Hmmm this is interesting. I am wondering whether running this outside of a transaction (autocommit mode) would do the trick. We already implement this for PostgreSQL where we autodetect the statements that cannot run within a transaction and automatically mark the migration non-transactional. What do you think?

@aebaugh
Copy link
Author

@aebaugh aebaugh commented Jul 12, 2017

That sounds like a great suggestion, wasn't aware of that hook in the statement builder. One thing I'm not sure about is DbMigrate appears to always wrap beforeMigrate in a TransactionTemplate. Unless that were changed to follow the same transaction detection, likely wouldn't fix this particular case.

Somewhat related, the transaction treatment of beforeMigrate vs e.g. beforeEachMigrate is different (this is understandable based on their scope, but initially unexpected). I think your suggestion would work as-is for beforeEachMigrate because it is called within the doMigrateGroup() method.

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Dec 18, 2017

Callbacks are currently always executed within a transaction. This is what causes this one to fail. The solution lies in extending the FlywayCallback interface to make it possible to determine whether a callback should be run in a transaction or not.

@aebaugh
Copy link
Author

@aebaugh aebaugh commented Dec 18, 2017

Wanted to add, a solution as simple as a configuration property e.g. flyway.executeCallbacksInTransaction that applied to all callbacks (or even migrations) for a particular flyway execution would work. That is, I don't see a need to be dynamic or detect which scripts should be executed within a transaction and which ones not, although that sounds like it would work as well. Interested to hear what the proposed fix may look like.

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Dec 18, 2017

We already have detection code available for regular migrations. This has proven to work well. So you can expect us to continue down this path.

@axelfontaine axelfontaine changed the title MS SQL Server SQL callbacks failing after upgrade to 4.2.0 Non-transactional SQL callbacks failing after upgrade to 4.2.0 Mar 1, 2018
axelfontaine added a commit to flyway/flywaydb.org that referenced this issue Mar 1, 2018
@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Mar 1, 2018

We ended up doing a major overhaul of the callback infrastructure as part of this fix. The nice added benefit is that it is now both cleaner and more powerful. Non-transactional statements are now automatically detected and cause the SQL callback to automatically run outside a transaction. The mixed property is also honored.

dohrayme pushed a commit to dohrayme/flyway that referenced this issue Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.