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

Session variables are leaking across migration scripts #216

Open
victor9000 opened this issue May 26, 2021 · 6 comments
Open

Session variables are leaking across migration scripts #216

victor9000 opened this issue May 26, 2021 · 6 comments
Labels

Comments

@victor9000
Copy link

victor9000 commented May 26, 2021

Problem

When a user defines a session variable, the expectation is that the variable will be scoped to the current migration script. However, the migrations seem to reuse the same session, resulting in scope leaks from one migration script to the next. You can recreate this problem using the following migration scripts:

  • migration_1
-- migrate:up
create table leak_demo_users (
  name varchar(255) not null
);
select 'VALUE_LEAK1' into @leaky_var1;
set @leaky_var2='VALUE_LEAK2';

-- migrate:down
drop table leak_demo_users;
  • migration_2
-- migrate:up
insert into
    leak_demo_users
values
    (@leaky_var1),
    (@leaky_var2)
;

-- migrate:down
delete from leak_demo_users;

These migrations will succeed if you run them in sequence, resulting in the following table records, which is not expected behavior.

mysql> select * from leak_demo_users;
+-------------+
| name        |
+-------------+
| VALUE_LEAK1 |
| VALUE_LEAK2 |
+-------------+

The expected behavior is for migration_2 to result in an error. However, if we perform a single rollback and run the migration again, then the correct behavior is encountered (see below). This is because now migration_2 is running in isolation without the benefit of the variable leak from migration_1.

Error: Error 1048: Column 'name' cannot be null

Problem Location

Here's location of the problematic function:

func (db *DB) migrate(drv Driver) error {

Solutions

A few ways to fix this include:

  • Close and reopen the database connection between each migration script
  • Provide a new CLI command step which will run only a single migration script and exit

The second option might be the most backwards compatible approach, since some folks may be dependent (perhaps unwittingly?) on this behavior.

@iby
Copy link

iby commented Jun 3, 2021

Wouldn't closing and reopening the connection affect the transactional execution though?

@victor9000
Copy link
Author

victor9000 commented Jun 3, 2021

Sure, but a single DDL statement in your migration scripts will have the same effect, no? If you start a transaction, insert some records, and follow that up with a create statement then the changes will be committed. So I don't think dbmate can make any guarantees on the effectiveness of running anything in a transaction, unless it starts enforcing a separation between DML and DDL migrations.

@amacneil
Copy link
Owner

amacneil commented Jun 5, 2021

I think the fresh connection for each migration solution sounds ok. Are there any other downsides to this approach?

We already open a fresh transaction for each migration, so I don’t think that is a blocker.

FYI - last time I checked, Postgres supports transactional DDL for most statements, but mysql does not.

@victor9000
Copy link
Author

Ah yes, it looks like you're correct regarding Postgres DDL transactions:
https://wiki.postgresql.org/wiki/Transactional_DDL_in_PostgreSQL:_A_Competitive_Analysis

The only other negative impact I can think of is the performance implications for users with a large number of migration scripts. I'm assuming that creating a new connection will take a bit more time than simply reusing an existing one.

@victor9000
Copy link
Author

victor9000 commented Jun 13, 2021

Perhaps the behavior of closing and reopening the connection can be controlled by a new cli flag? This way the default behavior is the same as before, meaning that the feature is backwards compatible. This way postgres users are not affected, and folks that want the new behavior can set the new flag. Say something like --multi-session?

@dossy dossy added the bug label Nov 19, 2023
@dossy
Copy link
Collaborator

dossy commented Nov 19, 2023

Just adding a note here so I don't forget:

At least for the Postgres driver, consider issuing a RESET ALL or DISCARD ALL in between migrations if the connection will be reused. I think this will avoid the network connection churn of establishing a new database connection for each migration, but clean up the state of the connection between migrations, which is the desired effect.

For MySQL, issuing a COM_RESET_CONNECTION between migrations may be a viable solution, except the underlying go-sql-driver/mysql driver we use hasn't implemented support for sending it.

This is a particularly gnarly issue for SQLite, because someone may want to use an in-memory database to test their migrations, and if we close and reopen the database in between migrations, each migration will be applied to a completely new in-memory database, which is not desirable. However, when testing against file-backed databases, it's important to reset session state between migrations to ensure that they are atomic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants