MJ Convento

/

Doctrine migrations, and why the Friday deploy broke

1,223 words, about 5 min read

The deploy script started at 17:01 yesterday. The new code went live at 17:16. In the fifteen minutes between those two events the application collected 213 of these, all identical:

PDOException: SQLSTATE[HY000]: General error: 1364
Field 'external_ref' doesn't have a default value

This is a client app on Symfony 2.8 with doctrine/doctrine-migrations-bundle and doctrine/migrations 1.5, MySQL 5.7.18, and a deploy script that does what almost every deploy script I have inherited does: install dependencies, run php app/console doctrine:migrations:migrate --no-interaction, then flip the symlink to the new release. Migrations first, code second. That order is the bug, and the migration that exposed it was one I generated and skimmed.

What the migration actually did

The entity got a new property, so I ran doctrine:migrations:diff, which compares your mapping metadata against the current database and writes the difference out as a migration class. Mine contained one line: ALTER TABLE invoice ADD external_ref VARCHAR(32) NOT NULL. On my laptop against a 400 row dump that runs in 30 milliseconds. On the client’s 3.1 million row table it ran for 7 minutes 40 seconds, and when it committed at 17:08 the release still serving traffic had never heard of the column.

Two documented things went wrong at once. Adding a column is an in place operation in InnoDB, but the online DDL table also marks it as rebuilding the table, which is why it took minutes rather than milliseconds. And the limitations page is blunt that an online DDL operation always needs an exclusive metadata lock in its final phase and must wait for transactions holding metadata locks to finish. A weekly report query was holding one. Everything queued behind the pending lock, so for part of that window the table was effectively unavailable to reads too.

The 1364 errors are the second half, and they start when the ALTER finishes rather than while it runs. A NOT NULL column with no explicit DEFAULT is exactly the case the data type defaults page covers: if an INSERT includes no value for the column and strict SQL mode is enabled, an error occurs for transactional tables and the statement is rolled back. Our connection runs with STRICT_TRANS_TABLES. The old code inserts invoices without external_ref, because as far as the old code is concerned there is no such column. Every invoice created between 17:08 and 17:16, while composer finished and the cache warmed and the symlink waited, failed.

One more thing to know before you trust the rollback story: doctrine/migrations wraps each migration in a transaction by default, and AbstractMigration::isTransactional() is what controls it. On MySQL that buys you nothing for schema changes, because DDL statements cause an implicit commit. ALTER TABLE is on that list. At 17:10 there was nothing to roll back, only something to write a second migration for.

A generated diff is a draft

I have been treating diff output as a deploy artifact. It is a starting point, and the bundle’s own documentation says what it is: a comparison of your mapping information against your actual database structure. It knows what your entities claim. It does not know how big the table is, how long the lock will last, what the running release expects, or whether the column should have a default. It also does not know that a table was created by hand outside the ORM, which is why the same diff wanted to drop one, and why schema_filter exists in the DBAL config.

Two options I should have been using for months. doctrine:migrations:migrate --dry-run executes the migration as a dry run, and --write-sql writes the SQL out to a file instead of executing it. Both are in the command’s own help text. Reading the file takes 20 seconds and would have caught a NOT NULL against a table I knew was large.

The other honesty problem is down(). A generated down() reverses the DDL, which is fine for a column that has only ever been empty, and a lie the moment the migration also moves data. AbstractMigration gives you throwIrreversibleMigrationException() for that case, plus abortIf() and skipIf() for guards. If a migration cannot truthfully undo itself, say so in code rather than leaving a down() that drops a column holding the only copy of something.

Expand and contract, in that order

The ordering rule I should have started with: a schema change has to be survivable by the code that is currently running, and a code change has to be survivable by the schema that is currently deployed. That means never one migration, and never the version I wrote. Add the column nullable, deploy code that writes it and tolerates nulls when reading, backfill in batches, then tighten the constraint, then remove the old path. Each step is separately deployable and each step leaves both the old and new code working.

The replacement, generated and then edited:

<?php

namespace ApplicationMigrations;

use DoctrineDBALMigrationsAbstractMigration;
use DoctrineDBALSchemaSchema;

class Version20170818181500 extends AbstractMigration
{
    public function up(Schema $schema)
    {
        $this->abortIf(
            $this->connection->getDatabasePlatform()->getName() !== 'mysql',
            'Written against MySQL 5.7 online DDL syntax.'
        );

        // Nullable on purpose. The release serving traffic right now
        // does not know this column exists, and its inserts omit it.
        $this->addSql(
            'ALTER TABLE invoice ADD external_ref VARCHAR(32) DEFAULT NULL, '
            . 'ALGORITHM=INPLACE, LOCK=NONE'
        );
    }

    public function down(Schema $schema)
    {
        $this->addSql('ALTER TABLE invoice DROP external_ref');
    }
}

The NOT NULL lives in a third migration, weeks later, after a backfill command has run and after the writer path is gone from every server. Even then it is not free: making a column NOT NULL rebuilds the table in place and needs strict mode to succeed, per the same online DDL table, so it gets the same treatment as any other long ALTER.

Our deploy script changed in two places. --write-sql now runs in CI on every branch that touches an entity, and the file goes in the merge request for a human to read. And migrations no longer run from the deploy script at all: expand migrations go out separately, by hand, on a weekday morning, before the code that needs them. The contract migration for external_ref is still unwritten, because the old writer is still in the codebase, and I would rather carry a nullable column for another month than repeat yesterday at five on a Friday.

Sources