Problem/Motivation

This code in Sql::ensureTables()

        try {
          $this->getDatabase()
            ->schema()
            ->createTable($this->mapTableName, $schema);
          break;
        }
        catch (DatabaseException $e) {
          $pdo_exception = $e->getPrevious();
          $mysql_index_error = $pdo_exception instanceof \PDOException && $pdo_exception->getCode() === '42000' && $pdo_exception->errorInfo[1] === 1071;
          $chunk_size--;
          // Rethrow the exception if the source IDs can not be in smaller
          // groups.
          if (!$mysql_index_error || $chunk_size <= 0) {
            throw $e;
          }
        }

assumes the DB driver is one implementing a PDO connection.

This blocks #3259709: Create the database driver for MySQLi that would be a non-PDO implementation.

Proposed resolution

Make that code db-driver abstract.

Implement two specific exceptions inheriting from SchemaException to cover the case of column too large and key too large, and throw them from mysql driver's ExceptionHandler. Change migrate's Sql to catch the new exceptions accordingly.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3389202

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mondrake created an issue. See original summary.

andypost’s picture

Issue tags: +blocker

mondrake’s picture

Status: Active » Needs review

Added a new exception (database independent), and changed Sql to use Connection::exceptionHandler that throws the new exception.

mondrake’s picture

Assigned: Unassigned » mondrake
Status: Needs review » Needs work

I'd like to add @group Database tests for the new exceptions.

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review

Added tests to mysql's SchemaTest

daffie’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record, +Needs issue summary update

We need a change record for the new exceptions. Also the IS needs to be updated for the new exceptions.

mondrake’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated IS

mondrake’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
daffie’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Can we add testing for PostgreSQL and SQLite.

The maximum character length for a text field in PostgreSQL is 10485760. See: https://www.postgresql.org/docs/current/datatype-character.html

The maximum character length for a text field in SQLite is 1 billion. See: https://www.sqlite.org/limits.html

The maximum number of columns in an index in PostgreSQL is 32. See: https://www.postgresql.org/docs/current/limits.html

The maximum number of columns in an index in SQLite is 2000. See: https://www.sqlite.org/limits.html

mondrake’s picture

Status: Needs work » Needs review

I suggest to do #10 in a follow up.

andypost’s picture

Status: Needs review » Needs work

I bet it just needs to extend existing tests for pgsql and sqlite like it was done for mysql

mondrake’s picture

Status: Needs work » Needs review

Well not extend, they have to be written specifically for each database since each database has different limitations, see #10.

One may argue that a test creating a table with over 2000 columns (sqlite) to trigger the failure is overkill. That’s why I suggest to do that in separate issues where each database tests can be discussed separately.

The blocker tag is only relevant for mysqli implementation.

andypost’s picture

@mondrake Thanks for explanation, it makes sense!

Moreover there's existing follow-up #3082239: Forbid limited length primary and unique keys, allow only in indexes

daffie’s picture

Status: Needs review » Reviewed & tested by the community

All code changes look good.
The IS and the CR are in order.
The testing for PostgreSQL and SQLite will be done in a followup.
For me it is RTBC.

mondrake’s picture

Filed followups for PostgreSQL and SQLite.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Very tidy, just adding a more explicit exception instead of relying on the exception message.

Committed df59482 and pushed to 11.x. Thanks!

  • catch committed df594822 on 11.x
    Issue #3389202 by mondrake, andypost, daffie: migrate\Plugin\migrate\...
catch’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.