Getting the following error when migrating D6->D8, since updating to mysql 5.7.9

SQLSTATE[42000]: Syntax error or access violation: 1171 All parts of a PRIMARY KEY must be NOT NULL; if you need NULL in a key, use UNIQUE instead: CREATE TABLE {migrate_map_5649cac9deb88} ( `sourceid1` VARCHAR(255) CHARACTER SET ascii COLLATE ascii_general_ci DEFAULT NULL, `source_row_status` TINYINT unsigned NOT NULL DEFAULT 0 COMMENT 'Indicates current status of the source row', `rollback_action` TINYINT unsigned NOT NULL DEFAULT 0 COMMENT 'Flag indicating what to do for this item on rollback', `last_imported` INT unsigned NOT NULL DEFAULT 0 COMMENT 'UNIX timestamp of the last time this row was imported', `hash` VARCHAR(64) NULL DEFAULT NULL COMMENT 'Hash of source row data, for detecting changes', PRIMARY KEY (`sourceid1`) ) ENGINE = InnoDB DEFAULT CHARACTER SET utf8mb4 COMMENT 'Mappings from source identifier value(s) to destination…'; Array ( )

Comments

peter.walter created an issue. See original summary.

peter.walter’s picture

StatusFileSize
new662 bytes

Declaring key as not null appears to fix.

mikeryan’s picture

Status: Active » Needs review

Set to "Needs review" for testing.

mikeryan’s picture

So, that test will at least demonstrate the change doesn't break anything with MySQL 5.5 - to demonstrate it fixes things for MySQL 5.7 we need #2616370: Testing MySQL >5.5?.

peter.walter’s picture

StatusFileSize
new1.1 KB

Last patch does not work reliably for new migrations, have moved 'not null' assignment outside of conditional branch.

peter.walter’s picture

StatusFileSize
new725 bytes

The last submitted patch, 5: migrate_fix_2616282-5.patch, failed testing.

webchick’s picture

Issue tags: +rc target triage
mikeryan’s picture

Priority: Normal » Major
mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Code is simple, straight-forward, and minimal risk at this point, and without it migration is totally broken on MySQL 5.7.9 (and I would guess 5.7.*). It'd be ideal to get a full test suite run on 5.7.9 of course (not least because who knows if there's anything else in core that would break?), but I'm perfectly comfortable with this patch.

webchick mentioned the idea of an upgrade path (setting not null on existing map table PKs) - but, if a site is already running 5.7.? then they wouldn't have managed to create any map tables to upgrade...

mikeryan’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

webchick mentioned the idea of an upgrade path (setting not null on existing map table PKs)

What happens if you have a site on 5.5 from before this patch lands, then upgrade to 5.7 if we don't provide an upgrade path?

catch’s picture

mikeryan’s picture

What happens if you have a site on 5.5 from before this patch lands, then upgrade to 5.7 if we don't provide an upgrade path?

Hmm... I would assume (hope) MySQL would fix the columns up on upgrade, because the alternative is it would break the tables. But, I don't know for sure, need to do some research.

mikeryan’s picture

Actually, as near as I can tell the current MySQL behavior is to automatically (and silently) make PK columns NOT NULL by default if it's not specified - the change in behavior is to now error out instead of silently fix it for you. So, any existing map tables already have their PK columns set NOT NULL.

catch’s picture

OK that's good to know. We should consider a follow-up to validate definitions in the database layer.

catch’s picture

Issue tags: -rc target triage +rc target

Given I made the parent critical, making this RC target too.

chx’s picture

Assigned: Unassigned » chx

May I ask for a few hours to read up on this?

chx’s picture

Status: Needs review » Needs work

My problem: what if the *source* has a null in one of the ids? It shouldn't, of course, of course, but it's the source, after all.

Mike Ryan says " In D7 the way to deal with that was to turn the NULL into 0/'' as appropriate for the field type in prepareRow".

I can agree on this approach but this needs to be documented on

  1. Source interface getIds
  2. hook_migrate_prepare_row
  3. Source prepareRow. While at it please add a @see hook_migrate_prepare_row() and whatever else we can think of, this doxygen is way too thin. Perhaps a separate issue.
chx’s picture

Assigned: chx » Unassigned
mikeryan’s picture

Status: Needs work » Needs review
StatusFileSize
new1.6 KB
new773 bytes

My problem: what if the *source* has a null in one of the ids? It shouldn't, of course, of course, but it's the source, after all.

In practice, this is rare - at least having NULL being a valid part of a unique ID on the source side (although a novice migration developer might make poor choices when defining source IDs and include a nullable field, that's not so rare).

Mike Ryan says " In D7 the way to deal with that was to turn the NULL into 0/'' as appropriate for the field type in prepareRow".

I can agree on this approach but this needs to be documented on

Source interface getIds

Yes, this is where it should be documented that source ID fields should never be NULL (and if a field that logically helps identify a source record as unique could be NULL, prepareRow() should rewrite it to 0/''). The attached patch improves the getIds() docblock (a quick edit as I'm at the end of my day, may bikeshedding commence...).

hook_migrate_prepare_row
Source prepareRow. While at it please add a @see hook_migrate_prepare_row() and whatever else we can think of, this doxygen is way too thin. Perhaps a separate issue.

Improving prepareRow() docs should be a separate issue, yes. I don't think this is the place to document the issue at hand - poorly-chosen source keys are something that can be fixed in prepareRow(), yes, but are by no means a primary (or even secondary) purpose for prepareRow().

mikeryan’s picture

And at the last second I recall, we can't fix up source keys in prepareRow(), to fix them up requires #2613878: Use hash for Migration source keys, rather than verbatim values.

Din-din time!

Status: Needs review » Needs work

The last submitted patch, 21: error_creating-2616282-21.patch, failed testing.

mikeryan’s picture

Status: Needs work » Needs review

WTF? Retest...

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

I tested this on MySQL 5.7.9 by running Drupal\file\Tests\Migrate\d6\MigrateFileTest. It fails without the patch and passes with it.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 50de1d6 on 8.0.x
    Issue #2616282 by peter.walter, mikeryan, chx, alexpott: error creating...
chx’s picture

Why was this rushed in? The doxygen now is factually incorrect as discussed in the child issue.

mikeryan’s picture

@chx: Can you point at the issue you're referencing? What doxygen is incorrect? Was there something that said you could have a NULL source key column?

It's important to note that the change has no actual effect for MySQL versions < 5.7.3 - the PK columns are automatically set to NOT NULL even if you don't explicitly set 'not null'.

chx’s picture

> if necessary, use prepareRow() or hook_migrate_prepare_row() to rewrite NULL values

The current architecture does not let you do that; there are no setters for source ID values at this moment.

mikeryan’s picture

Ah, right, should've gone back and fixed that up upon my realization in #22...

  • catch committed 50de1d6 on 8.1.x
    Issue #2616282 by peter.walter, mikeryan, chx, alexpott: error creating...

Status: Fixed » Closed (fixed)

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