yogthos / migratus

MIGRATE ALL THE THINGS!
642 stars 93 forks source link

Database locked when attempting to run coded migration #203

Closed freshhops closed 2 years ago

freshhops commented 3 years ago

Hi,

I have been having some issues trying to get coded migrations to work. I have a test where I'm trying to create a table

(ns test.migrations.test-migration
  (:require [test.db.core :as db]))

(defn test-migration [config]
  (println "+test-migration: " config)
  (println "result: " (db/test-create-test-table!))
  (println "-test-migration"))

However, it fails with an SQLite exception that the database file is locked. I was hoping to be able to use coded migrations for some of our more complex database migrations.

freshhops commented 3 years ago

It appears as if my database gets created, and the schema_migrations table also gets created. When it attempts to run my first up script though, it fails with the db locked error.

I've been following this guide: https://luminusweb.com/docs/migrations.html, as well as https://github.com/yogthos/migratus

yogthos commented 3 years ago

It sounds like SQLite db file is in use by a process when another is trying to modify it.

freshhops commented 3 years ago

I have thought about that, but then wouldn't it also fail for sql up and down files?

We have gotten those to work without issue. This is starting from a new instance of our application where the database has not been created yet.

yogthos commented 3 years ago

Not sure what the difference would be there. It does seem odd that it should work one way but not the other in terms of file locking.

freshhops commented 3 years ago

Would using the db connection from our db/core.clj to do inserts, and create tables be the cause?

Is this different from the connection used in the sql files? Or would it be the same?

yogthos commented 2 years ago

I suspect that trying to create multiple connections at the same time would be the problem. So, reusing a connection would probably be the way to go.

freshhops commented 2 years ago

Here's how I attempt to start my app:

(defn start-app [args]
  (mount/start #'test.config/env)
  (mount/start #'test.db.core/*db*)
  (migratus/migrate {:store :database
                      :db {:connection (.getConnection db/*db*)}}))
  ;; Also tried
  (migratus/migrate {:store :database
                     :db {:datasource db/*db*}})
 ;; and tried
  (migratus/migrate {:store :database
                     :db {:connection-uri "jdbc:sqlite:test_dev.db"}})
  ;; etc..
  )

(defn -main [& args] 
  (try 
    (start-app args)
    (catch Exception e
       (log/error e "Error starting app")
       (stop-app)
       (throw e))))

But I'm still getting a database locked error.

freshhops commented 2 years ago

It's interesting to note that the migration works when passing in the database information in like my previous comment, and that it only fails when introducing a coded migration (sql up migrations work).

I found I can also successfully call my db/test-create-test-table! method both before and after executing the migratus migration. It just seems to fall over during the actual migratus migration.

yogthos commented 2 years ago

Could be that there's some path that coded migration takes that doesn't release the file handle. Have you tried creating a connection for the migration, and then explicitly closing it after the migration completes?

freshhops commented 2 years ago

It's not failing to release the connection after migration though, it's failing during the coded migration whenever I try to write to the database.

freshhops commented 2 years ago

If I run db commands like create table before the migration, I am able to successfully create a table. It's only when trying to create a table through the coded migrations framework does it fail with DB locked.

Do you have any examples for coded migrations where you try to write something to the db?

I have a little test app I am willing to share that demonstrates the problem. https://github.com/freshhops/migratus-test

freshhops commented 2 years ago

I think it's because before running the migration, a db transaction is opened before the migrate-up occurs. When I try to interact with the db from within my application it then returns a lock error because that transaction that has been opened has not been committed.

Is there a way to say don't open a transaction for coded migrations?

freshhops commented 2 years ago

I think this is causing the issue:

;; up-fn and down-fn here are actually vars; invoking them as fns will deref
;; them and invoke the fn bound by the var.
(defrecord EdnMigration [id name up-fn down-fn]
  proto/Migration
  (id [this] id)
  (name [this] name)
  (tx? [this direction] true) ;; should this always be true?
  (up [this config]
    (when up-fn
      (up-fn config)))
  (down [this config]
    (when down-fn
      (down-fn config))))
yogthos commented 2 years ago

Ah yeah, I think you're correct. Making this togglable should fix the problem. Would you be ok to try do a pr for the fix?

freshhops commented 2 years ago

I can take a stab at it. Does this sound like a reasonable approach?

Add a new property to the .edn file, in addition to the current up and down function mappings. Something like:

{:ns migratus-test.db.migrations.test-migration
 :up-fn test-migration
 :down-fn nil
 :transaction? false}

I can keep the default as true for backwards compatability. So, if it's not specified, it'll default to true

yogthos commented 2 years ago

Yeah that looks pretty reasonable to me. 👍

freshhops commented 2 years ago

I think I need access to be able to push my topic branch. Could you help me with this?

yogthos commented 2 years ago

I could add you in, but you should be able to submit it as a pull request if you clone the repo. Once you commit to yours and push GitHub will show a button to make a pr.

freshhops commented 2 years ago

Github documentation says:

Note: To open a pull request in a public repository, you must have write access to the head or the source branch or, for organization-owned repositories, you must be a member of the organization that owns the repository to open a pull request.

Taken from: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request

yogthos commented 2 years ago

It's a bit misleading, the message at the top says:

Anyone with read access to a repository can create a pull request.

I've definitely contributed to repositories with just read access before. Is GitHub not showing your a pull request option on your fork?

freshhops commented 2 years ago

I'm just trying to push my local branch up and I see this:

image

yogthos commented 2 years ago

Ah, you have to fork the repo under your user first. Then you can make a pull request from your repository to this one. Use the fork button at the top right to do it:

image
freshhops commented 2 years ago

thanks, i'll give that a whirl!

yogthos commented 2 years ago

fixed by https://github.com/yogthos/migratus/pull/209