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

Support DDL transactions #123

Merged
merged 3 commits into from
Sep 5, 2023

Conversation

greg-rychlewski
Copy link
Contributor

Closes part of #87.

It doesn't seem to me like anything special needs to be done to support DDL transactions for SQLite. The tests were copied from Ecto SQL and seem to pass with no issue.

For the other part of the issue, locking to prevent concurrent migrations, I don't think anything can be done as is. Ecto requires a separate transaction to hold the lock, independent of the transaction used to perform the migration. This is to allow users the ability to lock even if their migration command cannot be run inside of a transaction.

From what I can tell this is not compatible with how locks work in SQLite. It seems the same transaction has to hold the lock and do the writing. There's no mutually exclusive lock that can be held that will allow a subsequent transaction to perform a DDL statement, from what I can see.

I don't think it's completely hopeless. But I just have to figure out a good proposal for Ecto for an option to allow everything to be in one transaction.

Copy link
Member

@warmwaffles warmwaffles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I just said sqlite couldn't support it because the upstream integration tests weren't able to pass. But this proves I was wrong.

@warmwaffles warmwaffles merged commit 0e7c7c3 into elixir-sqlite:main Sep 5, 2023
9 checks passed
@greg-rychlewski
Copy link
Contributor Author

greg-rychlewski commented Sep 5, 2023

I was thinking of a proposal to ecto sql to help with the locks for concurrent migrations. If I could get something like this accepted do you think you'd want to use it for this adapter?

Create a new configuration option called separate_lock_transaction/separate_migration_transaction

  • when true uses one transaction to lock for concurrent migrations and a different one to perform the migration (this is the current behaviour enforced by ecto sql for all adapters)
  • when false uses the same transaction to lock for concurrent migrations and perform the migration. this could be used by your adapter with begin exclusive transaction. and in your adapter if someone asks for migration locks without specifying this option as false can give a warning that the lock will be ignored and to set the option to false. right now the default behaviour of your adapter is to ignore the lock so don't want to start raising or anything like that.

@greg-rychlewski greg-rychlewski deleted the ddl_transaction branch September 5, 2023 22:32
@warmwaffles
Copy link
Member

That should work provided they have the database's journal opened with the wal set.

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

Successfully merging this pull request may close these issues.

2 participants