Turn on supportsTransactionalDdl for sqliteadapter

[sampbarrow]

SQLite does support ddl transactions, but supportsTransactionalDdl in the sqlite adapter returns false. I am doing it with my own adapter and it seems to work fine. Can this be set to true in the built in sqlite adapter? Would help a lot with migrations for anyone using sqlite.

[sampbarrow]

?

I didn't mean it as a command... just thought it might help others. I'm using my own driver anyway.

[koskimas]

Sorry about the comment I wrote yesterday. I was really drunk 😬. There's nothing wrong about this idea or in the way you presented it.

I'm not sure if SQLite really supports this for all possible DDL statements. If you can find proof it does , I can turn it on.

[sampbarrow]

Lol, no problem.

I can't find an official source, just comments on the internet, but it does seem to be widely agreed on. Here's a similar issue on another project.

https://github.com/golang-migrate/migrate/issues/346

I only realized on finding this comment this does create one potential issue though. Some pragma statements can not be used inside a transaction within sqlite (such as turning off foreign keys which is often done in migrations for sqlite since you often have to drop and re-create tables).

One solution would be to always turn off foreign keys before calling the migrator - but what if someone wants to leave foreign keys on for most migrations and only turn them off for some?

This would be ideal:

up(db: Kysely<any>) {
  db.raw("PRAGMA foreign_keys=0").execute();
  db.transaction().execute(...);
  db.raw("PRAGMA foreign_keys=1").execute();
}

But it doesn't work because it appears that the connection is locked by kysely before the migration (even currently when there is no transaction being used), so trying to create a transaction hangs forever.

Comments re transactional ddl:

https://intellipaat.com/community/16315/is-it-possible-to-roll-back-create-table-and-alter-table-statements-in-major-sql-databases https://stackoverflow.com/questions/29973010/upgrade-sqlite-database-with-transactions https://stackoverflow.com/questions/4692690/is-it-possible-to-roll-back-create-table-and-alter-table-statements-in-major-sql

[sampbarrow]

@koskimas Any thoughts on this? A simple flag when applying migrations for whether or not to use a locking connection would fix the problem. Not sure how easy that would be to implement? Then you could use transactions (or not use them) however you wanted within a migration.

[koskimas]

We should turn it on if it's supported. If it's not supported for all DDL statements, we definitely shouldn't turn it on. It's weird that there's no documentation about it.

Based on some sources sqlite commits the transaction when it encounters certain DDL statements. If that's true, it would be super confusing. Some migrations would roll back correctly, some wouldn't.

[sampbarrow]

Based on some sources sqlite commits the transaction when it encounters certain DDL statements.

I read that too but I think that might be for older versions only? But yes there doesn't seem to be a good source.

What do you think of the pragma issue? If we added the flag I mentioned that would allow a workaround for that without requiring us to turn on transactions universally for the adapter.

[koskimas]

A simple flag when applying migrations for whether or not to use a locking connection would fix the problem

That flag would be harmful on other dialects. Other dialects are able to use session/transaction level locks by using a single connection (session) through out the migration process. The flag would remove the single connection and use the pool instead.

Maybe I could make the transaction method work with a single connection 🤔. I'll see if that can be done

[sampbarrow]

Just to make sure we're clear, I'm suggesting an optional flag that would just never be used with the other dialects because no one would ever need it. Only sqlite (either on the adapter, or when you're running the migration, probably the latter). By default, current behavior would be used so it shouldn't be breaking, unless I'm misunderstanding something.

[koskimas]

I figured out a way to make this possible:

up(db: Kysely<any>) {
  db.raw("PRAGMA foreign_keys=0").execute();
  db.transaction().execute(...);
  db.raw("PRAGMA foreign_keys=1").execute();
}

Is that enough?

[sampbarrow]

Yep.

[koskimas]

It's now possible in 0.17.1. I'll still leave this open. Maybe we could add a flag for SqliteDialect and SqliteAdapter for enabling transactions by default.

[sampbarrow]

Great, I'll test in the next few days.

[sampbarrow]

It's now possible in 0.17.1. I'll still leave this open. Maybe we could add a flag for SqliteDialect and SqliteAdapter for enabling transactions by default.

Since sqlite has such limited support for alter table, I think it's going to be very common to want to turn off foreign keys in migrations. Others might, but personally I would not use this, how it works right now is perfect for me. Most of my migrations look like your original snippet. Has been working great by the way, thanks!

up(db: Kysely<any>) {
  db.raw("PRAGMA foreign_keys=0").execute();
  db.transaction().execute(...);
  db.raw("PRAGMA foreign_keys=1").execute();
}

[igalklebanov]

Issue seems to be stale. SQLite documentation is lacking in this department and the db itself covers transactional DDL partially it seems. An alternative approach was found to be working quite well.

Let's close this until there are new findings.