On current Drupal HEAD, schema.module's comparison report shows the following inconsistencies between the schema structures and the actual database following an update from D5 to D6:
1. db_log.variables: dblog_schema() declares this field as size 'medium' but system_update_6010() adds the field to the table as size 'big'. I'm pretty sure that 'big' is correct, so dblog_schema() should be updated.
2. profile_fields.name: profile_schema() declares this field to have default '' but profile.install:1.1 (D5's version) created the column as "default NULL" (i.e. no default, but nullable). As a result, after updating D5 to D6, there is a mismatch. Which is right? I think "no default" is semantically correct as you cannot have a profile field without a name, but I'm not certain.
3. node.vid: node_schema() declares node.vid as 'not null' => TRUE but system_update_6023() sets it to 'not null' => FALSE (by not specifying 'not null' at all) and has a comment that "vid is NULL". node_save first saves into node, gets the nid, then saves into node_revisions, gets the vid, then updates node.vid with the new vid. So node.vid could either be default 0 or nullable, but a vid of 0 is semantically invalid so NULL is probably right. Also, in http://drupal.org/node/149626, hunmonk specifically wrote system_update_6023() that way, so I'm guessing that nullable is right, though I'm certainly not sure.
These discrepancies are all easy to fix once we have an authoritative decision on what is correct.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | table_exists.patch | 1.13 KB | webernet |
| #6 | schema-inconsistency-173982-6.patch | 3.58 KB | bjaspan |
| #5 | no_seq_menu_1.patch | 1.87 KB | pwolanin |
| #3 | no_seq.patch | 708 bytes | chx |
Comments
Comment #1
bjaspan commentedAlso, chx removed the system update that added the sequences table on pgsql from my D6 update patch (I was busy getting married when the patch was finally committed so I could not argue against this decision which I think is a mistake). Therefore, the sequences table exists in the schema but not in pgsql. Bzzzz.
Comment #2
bjaspan commentedItems 2 and 3 were introduced by http://drupal.org/node/165766 and http://drupal.org/node/165775 by changing hook_schema() without a corresponding hook_update_N() function.
Comment #3
chx commentedComment #4
bjaspan commentedRe: the sequences table, chx and I discussed on IRC that the right solution is to remove it from system.schema since it is no longer part of Drupal's schema.
Comment #5
pwolanin commentedseems we lost at some point from system_update_6021 the code to drop {menu}. Adding that too (though I chx may argue jsut to leave it be).
Comment #6
bjaspan commentedThe attached patch fixes all problems mention in this issue:
1. watchdog.variables is added in system_update_6010 as size big, not medium, so as to match the schema.
2. The now-obsolete menu table is dropped. I have no opinion about whether this should stay or go. chx?
3. node.vid used to be NOT NULL. system_update_6023() previously explicitly made node.vid a nullable field. However, http://drupal.org/node/165766 explains why it can't be nullable. I just removed that part of 6023.
4. I removed sequences from system.schema. sequences will now be reported an as "extra" table on mysql instead of a "missing" table on pgsql (just as 'menu' would be missing on all databases if not for #2 above). IMHO, extra tables are not schema discrepancies that need to be fixed.
Comment #7
chx commentedat the end of the day, if you do not back up your database before update.php you might get what you deserve. drop the menu table.
Comment #8
chx commentedOne thing I am unsure -- whether this is critical. But the resulting schema is the same as for installed so it can't be wrong.
Comment #9
gábor hojtsyWhy don't we keep the menu table? We used to do this in previous versions (eg. the old_revision table used to stay to serve as a backup for stuff the revision update was not able to pick up). If the sequences table is kept to keep data which might be needed, then why not keep the menu table? This seems to be inconsistent to me.
Comment #10
pwolanin commented@Gabor - there is no code in Drupal that remains that can parse the {menu} table. I suppose it's possible that some contrib modules would want to be able to double-check their data from it during their update? I don't think any of us feel strongly that it must be dropped, it more an issue of not confusing admins/developers by leaving around unused tables.
Comment #11
gábor hojtsyWe used to leave around unused tables *if* there was a possibility that it contains data which was not possible to carry over in the upgrade. The old_revisions table was such an example.
Comment #12
chx commentedNo. That's not possible. We carry over everything from the menu table and then delete childrenless uncustomized items. And you need to back up your database before upgrade anyways.
Comment #13
chx commentedSo, it's good to go :)
So, non-customized items come from hook_menu and htey are handled by menu_rebuild so no need to deal with them specifically that's why we get rid of those.
Comment #14
gábor hojtsyThis clears it up for me. Committed, thanks.
Comment #15
pwolanin commentedI think bjaspan deserves most of the credit for this one.
Comment #16
webernet commentedThe profile module is not required, so the 'profile_fields' table may not exist, consequently we need to check that they exist before modifying it in system update 6032.
Comment #17
chx commentedTrivial but important.
Comment #18
gábor hojtsySure, thanks, committed.
Comment #19
(not verified) commented