Record search path failures on connect

Instead of ignoring them, record when the schema can't be added to the search path. Then use that value to avoid querying the database in `_sync_plan`. This prevents Sqitch from finding a valid project record when the search path contains a valid changes table and record for the project, which can happen when one starts using the current schema for the registry (the default on Oracle) and then tries to use a new registry. Resolves #668. To best make use of the new `_no_registry` attribute of Engine, rename the `initialized()` method to `_initialized()` and add a new `initialized()` method to check it before calling the engine-specific `_initialized()` method. Also rename the `initialize()` method to `_initialize()` and provide an `initialize()` implementation that sets `_no_registry` to false after a successful call to the engine-specific `_initialize()` method. Update the DBI `connected` callbacks to return anytime `$dbh->do` returns false, thereby always propagating errors. Previously errors were ignored in MySQL and Snowflake (and we had an invalid warehouse name in the tests that needed fixing!). For search path errors, call the new DBIEngine role `_handle_no_registry` method, which sets `_no_registry` to true and discards the error so that the DBI won't throw it once the callback function returns. Since the error handling is boolean-based, here, and the error handler is never called from inside the callback, remove the `try {}` block. It wasn't doing anything. While at it, require DBI 1.631 to ensure consistent behavior of error handling in callbacks and to eliminate a workaround for older versions in the mysql engine.
sqitchers · Jun 25, 2023 · c6ccc30 · c6ccc30
1 parent a814ddc
commit c6ccc30
Show file tree

Hide file tree

Showing 13 changed files with 108 additions and 76 deletions.
diff --git a/.github/workflows/snowflake.yml b/.github/workflows/snowflake.yml
@@ -29,5 +29,5 @@ jobs:
         env:
           PERL5LIB: "${{ github.workspace }}/local/lib/perl5"
           LIVE_SNOWFLAKE_REQUIRED: true
-          SQITCH_TEST_SNOWFLAKE_URI: db:snowflake://${{ secrets.SNOWFLAKE_USERNAME }}:${{ secrets.SNOWFLAKE_PASSWORD }}@sra81677.us-east-1/sqitchtest?Driver=Snowflake;warehouse=compute_wh
+          SQITCH_TEST_SNOWFLAKE_URI: db:snowflake://${{ secrets.SNOWFLAKE_USERNAME }}:${{ secrets.SNOWFLAKE_PASSWORD }}@sra81677.us-east-1/sqitchtest?Driver=Snowflake;warehouse=COMPUTE_WH
         run: prove -lvr t/snowflake.t
diff --git a/Changes b/Changes
@@ -38,6 +38,18 @@ Revision history for Perl extension App::Sqitch
        and is executable (and ends in `.exe` on Windows). Otherwise it simply
        returns `sqlplus` as before, assuming it will be found in the path.
        Thanks to @vectro for the suggestion (#747).
+     - Require DBI 1.631 or higher and remove MySQL engine workaround for
+       older versions.
+     - Detect missing registry schema on connect and don't bother trying to
+       query it when it does not exist. Done by adding an error handler to
+       DBIEngine.pm and calling it from the connection callback, and moving
+       the initialization queries into private methods, now called by
+       `initialized()` and `initialize()`. Fixes an issue where Sqitch might
+       find a project record in the current schema instead of the expected
+       registry schema. Thanks to @vectro for the report and investigation
+       (#668)!
+     - Fixed Snowflake and MySQL to properly raise errors on session query
+       failures immediately after connection.
 
 1.3.1  2022-10-01T18:49:30Z
      - Fixed a bug introduced in v1.3.0 where the Postgres engine would

diff --git a/dist/cpanfile b/dist/cpanfile
@@ -1,10 +1,10 @@
-# This file is generated by Dist::Zilla::Plugin::CPANFile v6.025
+# This file is generated by Dist::Zilla::Plugin::CPANFile v6.030
 # Do not edit this file directly. To change prereqs, edit the `dist.ini` file.
 
 requires "Algorithm::Backoff::Exponential" => "0.006";
 requires "Clone" => "0";
 requires "Config::GitLike" => "1.15";
-requires "DBI" => "0";
+requires "DBI" => "1.631";
 requires "DateTime" => "1.04";
 requires "DateTime::TimeZone" => "0";
 requires "Devel::StackTrace" => "1.30";

diff --git a/lib/App/Sqitch/Engine.pm b/lib/App/Sqitch/Engine.pm
@@ -145,6 +145,12 @@ has _locked => (
     default => 0,
 );
 
+has _no_registry => (
+    is      => 'rw',
+    isa     => Bool,
+    default => 0,
+);
+
 sub variables       { %{ shift->_variables }       }
 sub set_variables   {    shift->_variables({ @_ }) }
 sub clear_variables { %{ shift->_variables } = ()  }
@@ -977,7 +983,10 @@ sub _sync_plan {
     my $self = shift;
     my $plan = $self->plan;
 
-    if (my $state = $self->current_state) {
+    if ($self->_no_registry) {
+        # No registry found on connection, so no records in the database.
+        $plan->reset;
+    } elsif (my $state = $self->current_state) {
         my $idx = $plan->index_of($state->{change_id}) // hurl plan => __x(
             'Cannot find change {id} ({change}) in {file}',
             id     => $state->{change_id},
@@ -1312,11 +1321,23 @@ sub check {
 }
 
 sub initialized {
+    return 0 if $_[0]->_no_registry;
+    return $_[0]->_initialized;
+}
+
+sub _initialized {
     my $class = ref $_[0] || $_[0];
     hurl "$class has not implemented initialized()";
 }
 
 sub initialize {
+    my $self = shift;
+    $self->_initialize || return;
+    $self->_no_registry(0);
+    return $self;
+}
+
+sub _initialize {
     my $class = ref $_[0] || $_[0];
     hurl "$class has not implemented initialize()";
 }
@@ -2071,6 +2092,21 @@ The default implementation is effectively a no-op; consult the documentation
 for specific engines to determine whether they have implemented support for
 destination locking (by overriding C<try_lock()> and C<wait_lock()>).
 
+=head3 C<initialized>
+
+  $engine->initialize unless $engine->initialized;
+
+Returns true if the database has been initialized for Sqitch, and false if it
+has not.
+
+=head3 C<initialize>
+
+  $engine->initialize;
+
+Initializes the target database for Sqitch by installing the Sqitch registry
+schema and/or tables. Should be overridden by subclasses. This implementation
+throws an exception
+
 =head2 Abstract Instance Methods
 
 These methods must be overridden in subclasses.
@@ -2120,20 +2156,16 @@ This method is called after a change has been deployed or reverted and the
 logging of that change has failed. It should rollback changes started by
 C<begin_work>.
 
-=head3 C<initialized>
-
-  $engine->initialize unless $engine->initialized;
+=head3 C<_initialized>
 
 Returns true if the database has been initialized for Sqitch, and false if it
 has not.
 
-=head3 C<initialize>
-
-  $engine->initialize;
+=head3 C<_initialize>
 
 Initializes the target database for Sqitch by installing the Sqitch registry
 schema and/or tables. Should be overridden by subclasses. This implementation
-throws an exception
+throws an exception.
 
 =head3 C<register_project>
 

diff --git a/lib/App/Sqitch/Engine/exasol.pm b/lib/App/Sqitch/Engine/exasol.pm
@@ -146,11 +146,8 @@ has dbh => (
                     );
                     $dbh->do("ALTER SESSION SET TIME_ZONE='UTC'");
                     if (my $schema = $self->registry) {
-                        try {
-                            $dbh->do("OPEN SCHEMA $schema");
-                            # https://www.nntp.perl.org/group/perl.dbi.dev/2013/11/msg7622.html
-                            $dbh->set_err(undef, undef) if $dbh->err;
-                        };
+                        $dbh->do("OPEN SCHEMA $schema")
+                            or $self->_handle_no_registry($dbh);
                     }
                     return;
                 },
@@ -198,7 +195,7 @@ sub _multi_values {
     return join "\nUNION ALL ", ("SELECT $expr FROM dual") x $count;
 }
 
-sub initialized {
+sub _initialized {
     my $self = shift;
     return $self->dbh->selectcol_arrayref(q{
         SELECT EXISTS(
@@ -320,7 +317,7 @@ sub _registry_variable {
     return "DEFINE registry=$schema;";
 }
 
-sub initialize {
+sub _initialize {
     my $self   = shift;
     my $schema = $self->registry;
     hurl engine => __ 'Sqitch already initialized' if $self->initialized;

diff --git a/lib/App/Sqitch/Engine/firebird.pm b/lib/App/Sqitch/Engine/firebird.pm
@@ -184,7 +184,7 @@ sub is_deployed_tag {
     }, undef, $tag->id)->[0];
 }
 
-sub initialized {
+sub _initialized {
     my $self = shift;
 
     # Try to connect.
@@ -201,7 +201,7 @@ sub initialized {
     }, undef, 'CHANGES')->[0];
 }
 
-sub initialize {
+sub _initialize {
     my $self = shift;
     my $uri  = $self->registry_uri;
     hurl engine => __x(

diff --git a/lib/App/Sqitch/Engine/mysql.pm b/lib/App/Sqitch/Engine/mysql.pm
@@ -88,7 +88,7 @@ has dbh => (
             Callbacks             => {
                 connected => sub {
                     my $dbh = shift;
-                    $dbh->do("SET SESSION $_") for (
+                    $dbh->do("SET SESSION $_") or return for (
                         q{character_set_client   = 'utf8'},
                         q{character_set_server   = 'utf8'},
                         ($dbh->{mysql_serverversion} || 0 < 50500 ? () : (q{default_storage_engine = 'InnoDB'})),
@@ -104,15 +104,6 @@ has dbh => (
                             error_for_division_by_zero
                         )) . q{'},
                     );
-                    if (!$dbh->{mysql_serverversion} && DBI->VERSION < 1.631) {
-                        # Prior to 1.631, callbacks were inner handles and
-                        # mysql_* aren't set yet. So set InnoDB in a try block.
-                        try {
-                            $dbh->do(q{SET SESSION default_storage_engine = 'InnoDB'});
-                        };
-                        # https://www.nntp.perl.org/group/perl.dbi.dev/2013/11/msg7622.html
-                        $dbh->set_err(undef, undef) if $dbh->err;
-                    }
                     return;
                 },
             },
@@ -277,7 +268,7 @@ has initialized => (
     }
 );
 
-sub initialize {
+sub _initialize {
     my $self   = shift;
     hurl engine => __x(
         'Sqitch database {database} already initialized',

diff --git a/lib/App/Sqitch/Engine/oracle.pm b/lib/App/Sqitch/Engine/oracle.pm
@@ -117,11 +117,8 @@ has dbh => (
                         nls_timestamp_tz_format
                     );
                     if (my $schema = $self->registry) {
-                        try {
-                            $dbh->do("ALTER SESSION SET CURRENT_SCHEMA = $schema");
-                            # https://www.nntp.perl.org/group/perl.dbi.dev/2013/11/msg7622.html
-                            $dbh->set_err(undef, undef) if $dbh->err;
-                        };
+                        $dbh->do("ALTER SESSION SET CURRENT_SCHEMA = $schema")
+                            or $self->_handle_no_registry($dbh);
                     }
                     return;
                 },
@@ -265,7 +262,7 @@ sub is_deployed_change {
     )->[0];
 }
 
-sub initialized {
+sub _initialized {
     my $self = shift;
     return $self->dbh->selectcol_arrayref(q{
         SELECT 1
@@ -459,7 +456,7 @@ sub _registry_variable {
     );
 }
 
-sub initialize {
+sub _initialize {
     my $self   = shift;
     my $schema = $self->registry;
     hurl engine => __ 'Sqitch already initialized' if $self->initialized;

diff --git a/lib/App/Sqitch/Engine/pg.pm b/lib/App/Sqitch/Engine/pg.pm
@@ -137,14 +137,13 @@ has dbh => (
                 connected => sub {
                     my $dbh = shift;
                     $dbh->do('SET client_min_messages = WARNING');
-                    try {
-                        $dbh->do(
-                            'SET search_path = ?',
-                            undef, $self->registry
-                        );
-                        # https://www.nntp.perl.org/group/perl.dbi.dev/2013/11/msg7622.html
-                        $dbh->set_err(undef, undef) if $dbh->err;
-                    };
+                    # Setting search currently never fails, but call
+                    # _handle_no_registry in case that changes in the future.
+                    $dbh->do(
+                        'SET search_path = ?',
+                        undef, $self->registry
+                    ) or $self->_handle_no_registry($dbh);
+
                     # Determine the provider. Yugabyte says this is the right way to do it.
                     # https://yugabyte-db.slack.com/archives/CG0KQF0GG/p1653762283847589
                     my $v = $dbh->selectcol_arrayref(
@@ -198,7 +197,7 @@ sub _regex_op { '~' }
 
 sub _version_query { 'SELECT MAX(version)::TEXT FROM releases' }
 
-sub initialized {
+sub _initialized {
     my $self = shift;
     return $self->dbh->selectcol_arrayref(q{
         SELECT EXISTS(
@@ -208,7 +207,7 @@ sub initialized {
     }, undef, $self->registry, 'changes')->[0];
 }
 
-sub initialize {
+sub _initialize {
     my $self   = shift;
     hurl engine => __x(
         'Sqitch schema "{schema}" already exists',

diff --git a/lib/App/Sqitch/Engine/snowflake.pm b/lib/App/Sqitch/Engine/snowflake.pm
@@ -207,20 +207,18 @@ has dbh => (
             Callbacks         => {
                 connected => sub {
                     my $dbh = shift;
-                    try {
-                        my $wh = $dbh->quote_identifier($self->warehouse);
-                        my $role = $self->role;
-                        $dbh->do($_) for (
-                            ($role ? ("USE ROLE " . $dbh->quote_identifier($role)) : ()),
-                            "ALTER WAREHOUSE $wh RESUME IF SUSPENDED",
-                            "USE WAREHOUSE $wh",
-                            'USE SCHEMA ' . $dbh->quote_identifier($self->registry),
-                            'ALTER SESSION SET TIMESTAMP_TYPE_MAPPING=TIMESTAMP_LTZ',
-                            "ALTER SESSION SET TIMESTAMP_OUTPUT_FORMAT='YYYY-MM-DD HH24:MI:SS'",
-                            "ALTER SESSION SET TIMEZONE='UTC'",
-                        );
-                        $dbh->set_err(undef, undef) if $dbh->err;
-                    };
+                    my $wh = $dbh->quote_identifier($self->warehouse);
+                    my $role = $self->role;
+                    $dbh->do($_) or return for (
+                        ($role ? ("USE ROLE " . $dbh->quote_identifier($role)) : ()),
+                        "ALTER WAREHOUSE $wh RESUME IF SUSPENDED",
+                        "USE WAREHOUSE $wh",
+                        'ALTER SESSION SET TIMESTAMP_TYPE_MAPPING=TIMESTAMP_LTZ',
+                        "ALTER SESSION SET TIMESTAMP_OUTPUT_FORMAT='YYYY-MM-DD HH24:MI:SS'",
+                        "ALTER SESSION SET TIMEZONE='UTC'",
+                    );
+                    $dbh->do('USE SCHEMA ' . $dbh->quote_identifier($self->registry))
+                        or $self->_handle_no_registry($dbh);
                     return;
                 },
                 disconnect => sub {
@@ -286,7 +284,7 @@ sub _listagg_format {
 
 sub _ts_default { 'current_timestamp' }
 
-sub initialized {
+sub _initialized {
     my $self = shift;
     return $self->dbh->selectcol_arrayref(q{
         SELECT true
@@ -297,7 +295,7 @@ sub initialized {
      }, undef, $self->registry, 'changes')->[0];
 }
 
-sub initialize {
+sub _initialize {
     my $self   = shift;
     my $schema = $self->registry;
     hurl engine => __x(

diff --git a/lib/App/Sqitch/Engine/sqlite.pm b/lib/App/Sqitch/Engine/sqlite.pm
@@ -143,7 +143,7 @@ sub sqlite3 { @{ shift->_sqlite3 } }
 
 sub _version_query { 'SELECT CAST(ROUND(MAX(version), 1) AS TEXT) FROM releases' }
 
-sub initialized {
+sub _initialized {
     my $self = shift;
     return $self->dbh->selectcol_arrayref(q{
         SELECT EXISTS(
@@ -152,7 +152,7 @@ sub initialized {
     }, undef, 'changes')->[0];
 }
 
-sub initialize {
+sub _initialize {
     my $self   = shift;
     hurl engine => __x(
         'Sqitch database {database} already initialized',

diff --git a/lib/App/Sqitch/Engine/vertica.pm b/lib/App/Sqitch/Engine/vertica.pm
@@ -98,13 +98,8 @@ has dbh => (
             Callbacks         => {
                 connected => sub {
                     my $dbh = shift;
-                    try {
-                        $dbh->do(
-                            'SET search_path = ' . $dbh->quote($self->registry)
-                        );
-                        # https://www.nntp.perl.org/group/perl.dbi.dev/2013/11/msg7622.html
-                        $dbh->set_err(undef, undef) if $dbh->err;
-                    };
+                    $dbh->do('SET search_path = ' . $dbh->quote($self->registry))
+                        or $self->_handle_no_registry($dbh);
                     return;
                 },
             },
@@ -128,7 +123,7 @@ sub _client_opts {
     );
 }
 
-sub initialized {
+sub _initialized {
     my $self = shift;
     return $self->dbh->selectcol_arrayref(q{
         SELECT EXISTS(
@@ -137,7 +132,7 @@ sub initialized {
     }, undef, $self->registry)->[0];
 }
 
-sub initialize {
+sub _initialize {
     my $self   = shift;
     my $schema = $self->registry;
     hurl engine => __x(