Grinnz / Mojo-SQLite

Mojo::SQLite - A tiny Mojolicious wrapper for SQLite
https://metacpan.org/pod/Mojo::SQLite
Other
27 stars 12 forks source link

Fix another race-condition on initialization #26

Closed okurz closed 2 years ago

okurz commented 2 years ago

An exclusive access is needed to fix a race-condition of multiple processes starting up trying to create the database file.

This is a follow-up to 5cfc9c2.

Sorry, I forgot that part in https://github.com/Grinnz/Mojo-SQLite/pull/25

Grinnz commented 2 years ago

The default immediate transaction type is sufficient to obtain an immediate lock and avoid race conditions. I believe this is why I changed it to an immediate lock in https://github.com/Grinnz/Mojo-SQLite/commit/2603d397f08b66beda80ac6af47def483f80fb61

https://sqlite.org/lang_transaction.html

Grinnz commented 2 years ago

I believe the only difference would be preventing reads while the migration is underway. Do you know of a reason that would be helpful?

okurz commented 2 years ago

@kraih @Martchus @perlpunk our experiments proved that we need this to prevent the unique constraint problem when trying to create the database from two processes at the same time. Can you explain further why we need the "exclusive"?

Martchus commented 2 years ago

Judging by https://sqlite.org/lang_transaction.html the default transaction type is DEFERRED and not immediate.

Besides our experiment in which we ran into unique constraint problems I believe the problem is the following: The code does an insert which makes only sense if there was no mojo_migrations table before which is determined by a select in the beginning. However, without an exclusive transaction, another process might create the table and do the insert after we do the select and before we do the insert - so we do the insert based on the outdated assumption that there was no table.

perlpunk commented 2 years ago

If you look at https://github.com/mojolicious/mojo-pg/blob/main/lib/Mojo/Pg/Migrations.pm#L73 then you will also see an exclusive lock:

sub migrate {
...
  my $tx = $db->begin;
  $db->query('LOCK TABLE mojo_migrations IN EXCLUSIVE MODE');
  return $self if (my $active = $self->_active($db)) == $target;
Grinnz commented 2 years ago

Judging by https://sqlite.org/lang_transaction.html the default transaction type is DEFERRED and not immediate.

DBD::SQLite defaults to immediate.

Grinnz commented 2 years ago

Besides our experiment in which we ran into unique constraint problems I believe the problem is the following: The code does an insert which makes only sense if there was no mojo_migrations table before which is determined by a select in the beginning. However, without an exclusive transaction, another process might create the table and do the insert after we do the select and before we do the insert - so we do the insert based on the outdated assumption that there was no table.

This makes sense, thanks.

okurz commented 2 years ago

Could you please create a release tag and prepare a release in CPAN? That would be most appreciate so that we have the fix available in openSUSE for our openQA development. If not we would need to patch the openSUSE packages.