sqitch deploy does not revert after failed deployment to mysql

[mvgrimes]

Even when specified as command line options, mode=all and verify=true don't revert failed deployments. sqitch deploy seems to always report "ok" when deploying to mysql.

[~/src/kci/sqitch/kdb (✖)]• sqitch deploy --mode=all --verify
Deploying changes to local
  + lkono-fix-nfrc .. ERROR 1072 (42000) at line 5 in file: 'deploy/lkono-fix-nfrc.sql': Key column 'DefCode' doesn't exist in table
ERROR 1054 (42S22) at line 5 in file: 'verify/lkono-fix-nfrc.sql': Unknown column 'dontexist' in 'field list'
ok 

[~/src/kci/sqitch/kdb (✖)]• sqitch status
# On database local
# Project:  kdb
# Change:   d62a82c507baf2916c69c2d278ff8489bc2ce444
# Name:     lkono-fix-nfrc
# Deployed: 2015-01-27 18:13:51 -0500
# By:       Mark Grimes <mgrimes@cpan.org>
#
Nothing to deploy (up-to-date)

I would expect sqitch to revert the failed deploy and the lkono-fix-nfrc to be listed as not deployed.

The same thing happens when the deploy succeeds, but the sqlite.checkit() function fails:

[~/src/kci/sqitch/kdb (✖)]• sqitch deploy --mode=all --verify
Deploying changes to local
  + lkono-fix-nfrc .. ERROR 1644 (ERR0R) at line 6 in file: 'verify/lkono-fix-nfrc.sql': Fail!
ok
[~/src/kci/sqitch/kdb (✖)]• sqitch status
# On database local
# Project:  kdb
# Change:   d62a82c507baf2916c69c2d278ff8489bc2ce444
# Name:     lkono-fix-nfrc
# Deployed: 2015-01-27 18:29:23 -0500
# By:       Mark Grimes <mgrimes@cpan.org>
#
Nothing to deploy (up-to-date)

[theory]

Would you paste the contents of deploy/lkono-fix-nfrc.sql, or another example of a failing deploy script?

What version of MySQL are you using?

[mvgrimes]

So there are two different situations where I think sqitch should revert the deployment.

First, when the deploy fails:

[@pip:~/src/kci/sqitch/kdb (✖)]• cat deploy/deploy-fails.sql
-- Deploy deploy-fails

BEGIN;

  CREATE TABLE sqitch_test( ID integer ); -- This passes
  INSERT INTO sqitch_test VALUES ( 1, 2, 3 ); -- This fails

COMMIT;
[@pip:~/src/kci/sqitch/kdb (✖)]• cat revert/deploy-fails.sql
-- Revert deploy-fails

BEGIN;

  DROP TABLE sqitch_test;

COMMIT;
[@pip:~/src/kci/sqitch/kdb (✖)]• sqitch deploy --mode=all --verify
Deploying changes to local
  + deploy-fails .. ERROR 1136 (21S01) at line 6 in file: 'deploy/deploy-fails.sql': Column count doesn't match value count at row 1
ok
[@pip:~/src/kci/sqitch/kdb (✖)]• mysql kci_kdb -e "show tables like 'sqitch%';"
+-----------------------------+
| Tables_in_kci_kdb (sqitch%) |
+-----------------------------+
| sqitch_test                 |
+-----------------------------+

as you can see the INSERT statement in deploy/deploy-fails.sql generates an error, but the revert/deploy-fails script isn't run and the sqitch_test table remains.

And second, when the verify script fails:

[@pip:~/src/kci/sqitch/kdb (✖)]• cat deploy/verify-fails.sql
-- Deploy verify-fails

BEGIN;

  CREATE TABLE sqitch_test( ID integer ); -- This passes

COMMIT;
[@pip:~/src/kci/sqitch/kdb (✖)]• cat verify/verify-fails.sql
-- Verify verify-fails

BEGIN;

  SELECT NonExistantField FROM sqitch_test WHERE 0; -- Fails

ROLLBACK;
[@pip:~/src/kci/sqitch/kdb (✖)]• cat revert/verify-fails.sql
-- Revert verify-fails

BEGIN;

  DROP TABLE IF EXISTS sqitch_test;

COMMIT;
[@pip:~/src/kci/sqitch/kdb (✖)]• sqitch deploy --mode=all --verify
Deploying changes to local
  + verify-fails .. ERROR 1054 (42S22) at line 5 in file: 'verify/verify-fails.sql': Unknown column 'NonExistantField' in 'field list'
ok
[@pip:~/src/kci/sqitch/kdb (✖)]• sqitch status
# On database local
# Project:  kdb
# Change:   1fcef61cdcd16cd81597a81817a812ee4e6d546e
# Name:     verify-fails
# Deployed: 2015-01-28 13:29:50 -0500
# By:       Mark Grimes <mgrimes@cpan.org>
#
Nothing to deploy (up-to-date)
[@pip:~/src/kci/sqitch/kdb (✖)]• mysql kci_kdb -e "show tables like 'sqitch%';"
+-----------------------------+
| Tables_in_kci_kdb (sqitch%) |
+-----------------------------+
| sqitch_test                 |
+-----------------------------+

Again, revert/verify-fails.sql isn't run and the sqitch_test table remains.

I am actually running MariaDB:

[@pip:~/src/kci/sqitch/kdb (✖)]• mysql --version
mysql  Ver 15.1 Distrib 10.0.15-MariaDB, for Linux (x86_64) using readline 5.1

[Ovid]

Could this be an artifact of running MariaDB (e.g., MySQL+)? If MariaDB is the same as MySQL, DDL is not transaction safe. You can roll back data changes in a transaction, but not table changes. In PostgreSQL, DDL changes are transaction safe, so creating a new table would be rolled back if the transaction changes.

See also: https://wiki.postgresql.org/wiki/Transactional_DDL_in_PostgreSQL:_A_Competitive_Analysis

What this means is that for PostgreSQL, you shouldn't need to run the revert scripts, but in MySQL, you can't run the revert script because you don't know where in the deploy it failed. Sqitch is great, but the choice of underlying database makes it impossible to handle every use case.

Best, Ovid

[mvgrimes]

You are correct: DDL isn't transaction safe in either MySQL or MariaDB, so there is no rollback in the first example. Base on my reading of the docs for deploy --mode=all I still expected sqitch to run the revert script:

--mode = all In the event of failure, revert all deployed changes, back to the point at which deployment started. This is the default.

I could see how that might be read differently, depending on the definition of failure. I would rather have the revert scripts run on any failure and make sure to write them to cope with partial rollbacks (ie, DROP TABLE IF EXISTS).

Certainly, in the second example (where the deploy is successful, but the verify script fails) the revert script should run:

--verify Verify each change by running its verify script, if there is one. If a verify test fails, the deploy will be considered to have failed and the appropriate reversion will be carried out, depending on the value of --mode.

[theory]

Okay, when a deploy fails, Sqitch assumes that there is nothing to revert, so does not run the revert script. This makes sense if you think about it: Sqitch has no way of knowing how much succeeded and how much failed. This is why deploy scripts should be as atomic as possible, so that if they fail, nothing changes. On PostgreSQL, you can wrap most stuff in a transaction to get that functionality, but MySQL and other databases don't support transactional DDL. As a result, I recommend putting each DDL statement in a script by itself. In this example, CREATE TABLE should be in one deploy script, while the INSERT statement should be in another. If you have multiple transactional DML statements, you can put them into a single script, as long as you wrap them in a transaction that will rollback all changes.

As for the verify failing, I don't get the same behavior as you do:

> sqitch deploy --verify db:mysql://root@/failer
Deploying changes to db:mysql://root@/failer
  + deploy-fails .. ERROR 1054 (42S22) at line 5 in file: 'verify/deploy-fails.sql': Unknown column 'NonExistantField' in 'field list'
Verify script "verify/deploy-fails.sql" failed.
not ok
Deploy failed

> sqitch status db:mysql://root@/failer         
# On database db:mysql://root@/failer
No changes deployed

> mysql -u root failer -e 'show tables;'        

This is as I would expect: Nothing recorded as deployed, no sqitch_test table. What version of Sqitch do you have installed? I got the above results with both v0.997 and v0.998.

[theory]

Oh, I'm testing against MySQL 5.6.21. No idea how it might vary on MariaDB.

[mvgrimes]

I confirmed the behavior of verify on v0.997, v0.998 and git master (89f62f9), running on both Linux and OS/X.

[@pip:~/src/kci/sqitch/kdb (✖)]• sqitch deploy --verify
Deploying changes to local
  + verify-fails .. ERROR 1054 (42S22) at line 5 in file: 'verify/verify-fails.sql': Unknown column 'NonExistantField' in 'field list'
ok

I think I've tracked the issue to IPC::System::Simple. It doesn't seem to be throwing an exception. Where I expect something similar to:

[…/_others/sqitch/fail (✖)]• perlv IPC::System::Simple
IPC::System::Simple  - 1.25
[…/_others/sqitch/fail (✖)]• perl -MIPC::System::Simple=runx -E'runx("false")
'"false" unexpectedly returned exit value 1 at -e line 1.

Here is what the verify command runs:

[…/_others/sqitch/fail (✖)]• perl -MIPC::System::Simple=runx -E'runx(qw(mysql --user root --database kci_kdb --skip-pager --silent --skip-column-names --skip-line-numbers --execute), "source verify/verify-fails.sql")'
ERROR 1054 (42S22) at line 5 in file: 'verify/verify-fails.sql': Unknown column 'NonExistantField' in 'field list'

[theory]

Does MariaDB's mysql return non-0 on failure? What does this output?

mysql --user root --database kci_kdb --skip-pager --silent --skip-column-names --skip-line-numbers --execute "source verify/verify-fails.sql"
echo $?

[mvgrimes]

I just realized the same thing:

mysql --user root --execute "source verify/verify-fails.sql" && echo ok
ERROR 1046 (3D000) at line 5 in file: 'verify/verify-fails.sql': No
database selected
ok

It seems to be something specific to the source statement:

[…/_others/sqitch/fail (✖)]• mysql --user root --execute "select *;" &&
echo ok
ERROR 1096 (HY000) at line 1: No tables used

[theory]

Sounds like a bug in MariaDB's mysql statement…

[mvgrimes]

After all these years, I think I finally found a simple solution to this issue. Adding --abort-source-on-error to the mysql statement seems to produce the expected behavior.

I don't think this can be done as part of the sqitch config, but it is a one-line patch to has _mysql in App::Sqitch::Engine::mysql:

     push @ret, '--abort-source-on-error' if $self->dbh->{mysql_serverinfo} =~ /mariadb/i;

[theory]

Looks like maybe you could configure it in my.cnf, yes?

Happy to add it to mysql.pm similar to have you have it here, but can you tell in which version it was added? Would need to look something like:

--- a/lib/App/Sqitch/Engine/mysql.pm
+++ b/lib/App/Sqitch/Engine/mysql.pm
@@ -182,6 +182,10 @@ has _mysql => (
             '--skip-line-numbers',
         );

+        # Get Maria to abort properly on error.
+        push @ret, '--abort-source-on-error' if $dbh->{mysql_serverinfo} =~ /mariadb/i
+            && $dbh->{mysql_serverversion} >= 60000;
+
         # Add relevant query args.
         if (my @p = $uri->query_params) {
             my %option_for = (

Only with the proper version set.

[mvgrimes]

It looks like it was added Jul 9, 2008 with commit https://github.com/MariaDB/server/commit/7f02282404728d1da0990488b60c79fb53c9bf75 So it has been around for quite some time.

Looks like the next release after that was 5.0.66.

https://github.com/MariaDB/server/releases?after=clone-5.1.27-build

[theory]

So, is that 50066?

[theory]

@mvgrimes Can you confirm whether this patch addresses the issue?

diff --git a/lib/App/Sqitch/Engine/mysql.pm b/lib/App/Sqitch/Engine/mysql.pm
index d5f13f04..3d56fa5a 100644
--- a/lib/App/Sqitch/Engine/mysql.pm
+++ b/lib/App/Sqitch/Engine/mysql.pm
@@ -182,6 +182,15 @@ has _mysql => (
             '--skip-line-numbers',
         );

+        # Get Maria to abort properly on error.
+        my $vinfo = try { $self->sqitch->probe($self->client, '--version') } || '';
+        if ($vinfo =~ /mariadb/i) {
+            my ($version) = $vinfo =~ /Ver\s(\S+)/;
+            my ($maj, undef, $pat) = split /[.]/ => $version;
+            push @ret => '--abort-source-on-error'
+                if $maj > 5 || ($maj == 5 && $pat >= 66);
+        }
+
         # Add relevant query args.
         if (my @p = $uri->query_params) {
             my %option_for = (
diff --git a/t/mysql.t b/t/mysql.t
index eeb3cef9..78e3b1e8 100755
--- a/t/mysql.t
+++ b/t/mysql.t
@@ -104,6 +104,7 @@ my $mock_config = Test::MockModule->new('App::Sqitch::Config');
 $mock_config->mock(get => sub { $config{ $_[2] } });
 my $mysql_version = 'mysql  Ver 15.1 Distrib 10.0.15-MariaDB';
 $mock_sqitch->mock(probe => sub { $mysql_version });
+push @std_opts => '--abort-source-on-error';

 $target = App::Sqitch::Target->new(sqitch => $sqitch);
 ok $mysql = $CLASS->new(sqitch => $sqitch, target => $target),

Thanks!

[theory]

Would like to make a new release at the beginning of the year. I don't plan to include this change without confirmation that it fixed the problem, though. Thanks!

[mvgrimes]

Yes, I can verify that this works. When mysql fails it reports not ok and reverts happen as expected. Thanks!