rom-sql icon indicating copy to clipboard operation
rom-sql copied to clipboard

Possible race condition when inferring schema

Open paddor opened this issue 5 years ago • 6 comments

I have this DB:

sqlite3 current_cases.sqlite3 ".schema"
CREATE TABLE `current_cases` (`id` integer NOT NULL PRIMARY KEY AUTOINCREMENT, `path` varchar(255) NOT NULL UNIQUE, `object` blob NOT NULL);

The table was created by this relation's class method:

          class Relation < ROM::Relation[:sql]
            schema :current_cases, infer: true


            def self.create_current_cases_table(gateway)
              return if gateway.schema.include? :current_cases

              gateway.create_table(:current_cases) do
                primary_key :id
                column      :path,         String,   null: false, unique: true
                column      :object,       :blob,    null: false
              end
            end

Given this repo:

          class Repo < ROM::Repository[:current_cases]
            def delete_cases(paths)
              current_cases.by_path(paths).delete
            end
          end

Sometimes Relation#by_path is defined, sometimes it's not. Could there be a race condition?

Unfortunately it only happens in production and I can't reproduce it in a standalone script on my development machine.

  • Affects my production application: YES
  • Ruby version: 2.7
  • OS: Ubuntu 18.04

paddor avatar Jun 16 '20 14:06 paddor

@paddor I can see that you have a method that creates the table in the relation class - this is probably the culprit because if you load relation classes without tables in the db, then inferring schemas can fail. Try moving this method to a different place. In general do not load relations and finalize rom setup if there are no tables yet. You can have db management tasks (create, drop, migrate etc.) with just rom configuration instantiated - there's no need for a finalized rom container.

solnic avatar Jun 17 '20 05:06 solnic

@solnic I'm not sure if I understand correctly. I'm assuming by loading you mean the registration of the class in the config? The table creation is in a class method, called after creating the config and before creating a ROM container.

This is how the config and the container are instanciated:

          def initialize(uri)
            @config = ROM::Configuration.new :sql, uri do |config|
              config.register_relation Relation
              config.default.use_logger log
            end
          end


          def setup
            Relation.create_current_cases_table @config.gateways[:default]

            @container = ::ROM.container @config
            @repo = Repo.new @container
          end

#setup is called after initialization.

Should @config.register_relation Relation be done after creating the table?

paddor avatar Jun 17 '20 19:06 paddor

The thing is that this error (failed to infer DB schema and thus methods like #by_path missing) also occured when the table already existed.

paddor avatar Jun 17 '20 19:06 paddor

I'm assuming by loading you mean the registration of the class in the config?

Yes

The thing is that this error (failed to infer DB schema and thus methods like #by_path missing) also occured when the table already existed.

OK then disregard what I wrote. This is a tough one especially that there's no reproduction script. Auto-restriction methods are defined in the 'configuration.relations.schema.set' event which is triggered after a relation schema was established - this means there should be a table ready.

Notice that these methods are only defined for indexed columns - are you sure indices are defined in the db?

solnic avatar Jun 18 '20 07:06 solnic

This was the DB schema:

sqlite3 current_cases.sqlite3 ".schema"
CREATE TABLE `current_cases` (`id` integer NOT NULL PRIMARY KEY AUTOINCREMENT, `path` varchar(255) NOT NULL UNIQUE, `object` blob NOT NULL);

Sometimes it worked, sometimes it didn't. The schema did not change.

I don't know if SQLite creates indices for PRIMARY KEY and UNIQUE columns.

paddor avatar Jun 22 '20 15:06 paddor

@paddor yes it does. Would it be possible for you to create a reproduction script that mimics your prod setup?

solnic avatar Jun 23 '20 04:06 solnic