Skip to content

Commit

Permalink
Use database name in MySQL lock
Browse files Browse the repository at this point in the history
So that the scope of the lock is limited only to that database. This is
probably the most intuitive way to do it, in that it's probably what the user
expects. Preventing parallel deploys to different databases is more confusing.
Resolves #670.
  • Loading branch information
theory committed Aug 28, 2022
1 parent fd94b4f commit 914b44c
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 14 deletions.
5 changes: 4 additions & 1 deletion Changes
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
Revision history for Perl extension App::Sqitch

1.3.1

- Fixed a bug introduced in v1.3.0 where the Postgres engine would
always pass the port to `psql`, thus ignoring the `PGPORT` environment
variable. Thanks to Cam Feenstra for the spot (#675)!
- Fixed test failures on OSes where the local time zone cannot be
determined. Thanks to Slaven Rezić for the test reports, and to
Dave Rolsky for the solution (#672).
- Updated the MySQL deploy/revert lock to be specific to the target
database. This will allow multiple instances of Sqitch to run at the
same time as long as they're connecting to different databases. Thanks
to Dmytro Kh for the report and discussion of the options (#670).

1.3.0 2022-08-12T22:09:13Z
- Fixed an issue when testing Firebird on a host with Firebird installed
Expand Down
25 changes: 18 additions & 7 deletions lib/App/Sqitch/Engine/mysql.pm
Original file line number Diff line number Diff line change
Expand Up @@ -313,24 +313,35 @@ sub begin_work {
return $self;
}

# Override to try to acquire a lock on the string "sqitch working" without
# waiting.
# We include the database name in the lock name because that's probably the most
# stringent lock the user expects. Locking the whole server with a static string
# prevents parallel deploys to other databases. Yes, locking just the target
# allows parallel deploys to conflict with one another if they make changes to
# other databases, but is not a great practice and likely an anti-pattern. So
# stick with the least surprising behavior.
# https://github.com/sqitchers/sqitch/issues/670
sub _lock_name {
'sqitch working on ' . shift->uri->dbname
}

# Override to try to acquire a lock on the string "sqitch working on $dbname"
# without waiting.
sub try_lock {
my $self = shift;
# Can't create a lock in the registry if it doesn't exist.
$self->initialize unless $self->initialized;
$self->dbh->selectcol_arrayref(
q{SELECT get_lock('sqitch working', 0)}
q{SELECT get_lock(?, ?)}, undef, $self->_lock_name, 0,
)->[0]
}

# Override to try to acquire a lock on the string "sqitch working", waiting
# for the lock until timeout.
# Override to try to acquire a lock on the string "sqitch working on $dbname",
# waiting for the lock until timeout.
sub wait_lock {
my $self = shift;
$self->dbh->selectcol_arrayref(
q{SELECT get_lock('sqitch working', ?)},
undef, $self->lock_timeout
q{SELECT get_lock(?, ?)}, undef,
$self->_lock_name, $self->lock_timeout,
)->[0]
}

Expand Down
19 changes: 13 additions & 6 deletions t/mysql.t
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ is_deeply [$CLASS->config_vars], [
client => 'any',
], 'config_vars should return three vars';

my $config = TestConfig->new('core.engine' => 'mysql');
my $uri = URI::db->new('db:mysql:mydb');
my $config = TestConfig->new(
'core.engine' => 'mysql',
'engine.mysql.target' => $uri->as_string,
);
my $sqitch = App::Sqitch->new(config => $config);
my $target = App::Sqitch::Target->new(sqitch => $sqitch);
isa_ok my $mysql = $CLASS->new(sqitch => $sqitch, target => $target), $CLASS;
Expand All @@ -51,7 +55,6 @@ is $mysql->key, 'mysql', 'Key should be "mysql"';
is $mysql->name, 'MySQL', 'Name should be "MySQL"';

my $client = 'mysql' . (App::Sqitch::ISWIN ? '.exe' : '');
my $uri = URI::db->new('db:mysql:');
is $mysql->client, $client, 'client should default to mysql';
is $mysql->registry, 'sqitch', 'registry default should be "sqitch"';
my $sqitch_uri = $uri->clone;
Expand All @@ -60,6 +63,8 @@ is $mysql->registry_uri, $sqitch_uri, 'registry_uri should be correct';
is $mysql->uri, $uri, qq{uri should be "$uri"};
is $mysql->registry_destination, 'db:mysql:sqitch',
'registry_destination should be the same as registry_uri';
is $mysql->_lock_name, 'sqitch working on ' . $uri->dbname,
'_lock_name should be correct';

my @std_opts = (
(App::Sqitch::ISWIN ? () : '--skip-pager' ),
Expand All @@ -78,6 +83,7 @@ if ($vinfo =~ /mariadb/i) {
my $mock_sqitch = Test::MockModule->new('App::Sqitch');
my $warning;
$mock_sqitch->mock(warn => sub { shift; $warning = [@_] });
$mysql->uri->dbname('');
is_deeply [$mysql->mysql], [$client, '--user', $sqitch->sysuser, @std_opts],
'mysql command should be user and std opts-only';
is_deeply $warning, [__x
Expand Down Expand Up @@ -661,16 +667,17 @@ DBIEngineTest->run(
like $sql_mode, qr/\b\Q$mode\E\b/i, "sql_mode should include $mode";
}
},
lock_sql => sub { return {
is_locked => q{SELECT is_used_lock('sqitch working')},
try_lock => q{SELECT get_lock('sqitch working', 0)},
lock_sql => sub {
my $lock_name = shift->_lock_name; return {
is_locked => "SELECT is_used_lock('$lock_name')",
try_lock => "SELECT get_lock('$lock_name', 0)",
wait_time => 1, # get_lock() does not support sub-second precision, apparently.
async_free => 1,
free_lock => 'SELECT ' . ($dbh ? do {
# MySQL 5.5-5.6 and Maria 10.0-10.4 prefer release_lock(), while
# 5.7+ and 10.5+ prefer release_all_locks().
$dbh->selectrow_arrayref('SELECT version()')->[0] =~ /^(?:5\.[56]|10\.[0-4])/
? q{release_lock('sqitch working')}
? "release_lock('$lock_name')"
: 'release_all_locks()'
} : ''),
} },
Expand Down

0 comments on commit 914b44c

Please sign in to comment.