search

Daursu / laravel-zero-downtime-migration

Zero downtime migrations with Laravel and pt-online-schema-change
MIT License
80 stars 13 forks source link

Use the default database connection for doctrine #31

Closed dbhynds closed 2 years ago

dbhynds commented 2 years ago

We ran into an error while trying to modify a column in our database. It seems that the PDO object used by pt-osc connection is incompatible with the doctrine connection. This PR changes the BaseConnection to use the default database connection as the doctrine connection instead of the zero-downtime one.

The root of the problem was basically that the arguments provided to the PDO constructor for the doctrine connection are reused from the PtOnlineSchemaChangeConnection arguments. However, the two interfaces are not compatible. (See expected vs actual arguments below.) See: https://www.php.net/manual/en/ref.pdo-mysql.php

I'm not 100% positive this is the best way to solve this problem, but it does work. I feel like it would be better to be able to specify which DB connection to use for doctrine, instead of just assuming the default connection, but I wasn't sure the best way to go about that. I'm open to suggestions.

More details:

The migration we were running:

ZeroDowntimeSchema::table('images', function (Blueprint $table) {
    $table->uuid('post_id')->nullable()->change();
});

The exception we were getting:

   TypeError 

  Attribute value must be of type int for selected attribute, string given

  at vendor/laravel/framework/src/Illuminate/Database/Connectors/Connector.php:70
     66▕         if (class_exists(PDOConnection::class) && ! $this->isPersistentConnection($options)) {
     67▕             return new PDOConnection($dsn, $username, $password, $options);
     68▕         }
     69▕ 
  ➜  70▕         return new PDO($dsn, $username, $password, $options);
     71▕     }
     72▕ 
     73▕     /**
     74▕      * Determine if the connection is persistent.

      +15 vendor frames 
  16  database/migrations/2022_07_28_150549_test.php:26
      Daursu\ZeroDowntimeMigration\ZeroDowntimeSchema::table("images", Object(Closure))

The relevant clues in the from the stack trace:

vendor/laravel/framework/src/Illuminate/Database/Connection.php:1014 - Illuminate\Database\Connection::getPdo()
vendor/laravel/framework/src/Illuminate/Database/Connection.php:994 - Illuminate\Database\Connection::getDoctrineConnection()
vendor/laravel/framework/src/Illuminate/Database/Schema/Grammars/ChangeColumn.php:36 - Illuminate\Database\Connection::getDoctrineSchemaManager()

Expected arguments for the pdo closure:

[
  8 => 0
  3 => 2
  11 => 0
  17 => false
  20 => false
  2 => 2
  12 => false
  1014 => true
]

Actual arguments for the pdo closure:

[
  17 => false
  20 => false
  0 => "--print"
  1 => "--set-vars=sql_mode='NO_ENGINE_SUBSTITUTION\,ALLOW_INVALID_DATES'"
  2 => "--no-check-foreign-keys"
  3 => "--no-check-unique-key-change"
  4 => "--no-check-alter"
  5 => "--alter-foreign-keys-method=auto"
  6 => "--recursion-method=none"
  7 => "--pause-file=/tmp/pause-schema-change"
  8 => "--max-load=Threads_running=50"
  9 => "--critical-load=Threads_running=100"
  10 => "--tries=create_triggers:10:60,update_foreign_keys:10:60,analyze_table:10:60,swap_tables:10:60,drop_triggers:10:60"
  11 => ""
  12 => "--drop-old-table"
]

The full stack trace:

vendor/laravel/framework/src/Illuminate/Database/Connectors/Connector.php:72 - PDO::__construct(/** redacted **/)
vendor/laravel/framework/src/Illuminate/Database/Connectors/Connector.php:46 - Illuminate\Database\Connectors\Connector::createPdoConnection(/** redacted **/)
vendor/laravel/framework/src/Illuminate/Database/Connectors/MySqlConnector.php:24 - Illuminate\Database\Connectors\Connector::createConnection(/** redacted **/)
vendor/laravel/framework/src/Illuminate/Database/Connectors/ConnectionFactory.php:184 - Illuminate\Database\Connectors\MySqlConnector::connect()
vendor/laravel/framework/src/Illuminate/Database/Connection.php:1064 - Illuminate\Database\Connectors\ConnectionFactory::Illuminate\Database\Connectors\{closure}()
vendor/laravel/framework/src/Illuminate/Database/Connection.php:1064 - call_user_func(Object(Closure))
vendor/laravel/framework/src/Illuminate/Database/Connection.php:1014 - Illuminate\Database\Connection::getPdo()
vendor/laravel/framework/src/Illuminate/Database/Connection.php:994 - Illuminate\Database\Connection::getDoctrineConnection()
vendor/laravel/framework/src/Illuminate/Database/Schema/Grammars/ChangeColumn.php:36 - Illuminate\Database\Connection::getDoctrineSchemaManager()
vendor/laravel/framework/src/Illuminate/Database/Schema/Grammars/Grammar.php:83 - Illuminate\Database\Schema\Grammars\ChangeColumn::compile(Object(Illuminate\Database\Schema\Grammars\MySqlGrammar), Object(Illuminate\Database\Schema\Blueprint), Object(Illuminate\Support\Fluent), Object(Daursu\ZeroDowntimeMigration\Connections\PtOnlineSchemaChangeConnection))
vendor/laravel/framework/src/Illuminate/Database/Schema/Blueprint.php:135 - Illuminate\Database\Schema\Grammars\Grammar::compileChange(Object(Illuminate\Database\Schema\Blueprint), Object(Illuminate\Support\Fluent), Object(Daursu\ZeroDowntimeMigration\Connections\PtOnlineSchemaChangeConnection))
vendor/laravel/framework/src/Illuminate/Database/Schema/Blueprint.php:108 - Illuminate\Database\Schema\Blueprint::toSql(Object(Daursu\ZeroDowntimeMigration\Connections\PtOnlineSchemaChangeConnection), Object(Illuminate\Database\Schema\Grammars\MySqlGrammar))
vendor/laravel/framework/src/Illuminate/Database/Schema/Builder.php:364 - Illuminate\Database\Schema\Blueprint::build(Object(Daursu\ZeroDowntimeMigration\Connections\PtOnlineSchemaChangeConnection), Object(Illuminate\Database\Schema\Grammars\MySqlGrammar))
vendor/laravel/framework/src/Illuminate/Database/Schema/Builder.php:211 - Illuminate\Database\Schema\Builder::build(Object(Illuminate\Database\Schema\Blueprint))
vendor/daursu/laravel-zero-downtime-migration/src/ZeroDowntimeSchema.php:51 - Illuminate\Database\Schema\Builder::table("images", Object(Closure))
database/migrations/2022_07_28_150549_test.php:26 - Daursu\ZeroDowntimeMigration\ZeroDowntimeSchema::table("images", Object(Closure))
vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:472 - Test::up()
vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:394 - Illuminate\Database\Migrations\Migrator::runMethod(Object(Packback\Infrastructure\Services\MySqlConnection), Object(Test), "up")
vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:403 - Illuminate\Database\Migrations\Migrator::Illuminate\Database\Migrations\{closure}()
vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:202 - Illuminate\Database\Migrations\Migrator::runMigration(Object(Test), "up")
vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:167 - Illuminate\Database\Migrations\Migrator::runUp("/var/www/database/migrations/2022_07_28_150549_test.php")
vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:112 - Illuminate\Database\Migrations\Migrator::runPending([])
vendor/laravel/framework/src/Illuminate/Database/Console/Migrations/MigrateCommand.php:86 - Illuminate\Database\Migrations\Migrator::run([])
vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:606 - Illuminate\Database\Console\Migrations\MigrateCommand::Illuminate\Database\Console\Migrations\{closure}()
vendor/laravel/framework/src/Illuminate/Database/Console/Migrations/MigrateCommand.php:98 - Illuminate\Database\Migrations\Migrator::usingConnection(Object(Closure))
vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php:36 - Illuminate\Database\Console\Migrations\MigrateCommand::handle()
vendor/laravel/framework/src/Illuminate/Container/Util.php:40 - Illuminate\Container\BoundMethod::Illuminate\Container\{closure}()
vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php:93 - Illuminate\Container\Util::unwrapIfClosure(Object(Closure))
vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php:37 - Illuminate\Container\BoundMethod::callBoundMethod(Object(Illuminate\Foundation\Application), Object(Closure))
vendor/laravel/framework/src/Illuminate/Container/Container.php:653 - Illuminate\Container\BoundMethod::call(Object(Illuminate\Foundation\Application), [])
vendor/laravel/framework/src/Illuminate/Console/Command.php:136 - Illuminate\Container\Container::call()
vendor/symfony/console/Command/Command.php:298 - Illuminate\Console\Command::execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Illuminate\Console\OutputStyle))
vendor/laravel/framework/src/Illuminate/Console/Command.php:121 - Symfony\Component\Console\Command\Command::run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Illuminate\Console\OutputStyle))
vendor/symfony/console/Application.php:1024 - Illuminate\Console\Command::run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
vendor/symfony/console/Application.php:299 - Symfony\Component\Console\Application::doRunCommand(Object(Illuminate\Database\Console\Migrations\MigrateCommand), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
vendor/symfony/console/Application.php:171 - Symfony\Component\Console\Application::doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
vendor/laravel/framework/src/Illuminate/Console/Application.php:94 - Symfony\Component\Console\Application::run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php:129 - Illuminate\Console\Application::run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
artisan:37 - Illuminate\Foundation\Console\Kernel::handle(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
Daursu commented 2 years ago

Looking further into it, this seems to be caused by using the name options when configuring the additional parameters to be passed to pt-online-schema-change command. The laravel framework pulls any parameters under options array and passes them down to PDO. If I rename the property options to params then the issue goes away.

For example

'connections' => [
    'zero-downtime' => [
        'driver' => 'pt-online-schema-change',

        // This is your master write access database connection details
        'host' => env('DB_HOST', '127.0.0.1'),
        'port' => env('DB_PORT', '3306'),
        'database' => env('DB_DATABASE', 'forge'),
        'username' => env('DB_USERNAME', 'forge'),
        'password' => env('DB_PASSWORD', ''),

        // Additional options, depending on your setup
        // all options available here: https://www.percona.com/doc/percona-toolkit/LATEST/pt-online-schema-change.html
        'options' => [
            '--nocheck-replication-filters',
            '--nocheck-unique-key-change',
            '--recursion-method=none',
            '--chunk-size=2000',
        ],
    ],
],

changed to 

'connections' => [
    'zero-downtime' => [
        'driver' => 'pt-online-schema-change',

        // This is your master write access database connection details
        'host' => env('DB_HOST', '127.0.0.1'),
        'port' => env('DB_PORT', '3306'),
        'database' => env('DB_DATABASE', 'forge'),
        'username' => env('DB_USERNAME', 'forge'),
        'password' => env('DB_PASSWORD', ''),

        // Additional options, depending on your setup
        // all options available here: https://www.percona.com/doc/percona-toolkit/LATEST/pt-online-schema-change.html
        'params' => [
            '--nocheck-replication-filters',
            '--nocheck-unique-key-change',
            '--recursion-method=none',
            '--chunk-size=2000',
        ],
    ],
],

This would be a breaking change as anyone using this package would have to rename the config parameter. I will try and get this fixed in the next major release.

Daursu commented 2 years ago

Please see the fix for this bug in #32. Thank you for raising this issue.