node.nid is an unsigned int, and history.nid is only an int. If node.nid manages to get big enough, there will no longer be history. A customer recently ran into this due to a high auto_increment_increment in MySQL combined with a lot of rollbacks.
max int(11) is 2147483647
max int(10) unsigned is 4294967295
therefore, you can have more node.nid than you can contain in history.nid
This bug appears to be in both 7.x and 8.x - and it should be a simple patch. If patching is undesirable, then this should be sufficient:
ALTER TABLE history
MODIFY nid INT(11) UNSIGNED NOT NULL DEFAULT '0';
Supporting info:
table structures of a d8 site and a d7 site - node and history tables
mysql [d8site]> show create table history\G
*************************** 1. row ***************************
Table: history
Create Table: CREATE TABLE `history` (
`uid` int(11) NOT NULL DEFAULT '0' COMMENT 'The users.uid that read the node nid.',
`nid` int(11) NOT NULL DEFAULT '0' COMMENT 'The node.nid that was read.',
`timestamp` int(11) NOT NULL DEFAULT '0' COMMENT 'The Unix timestamp at which the read occurred.',
PRIMARY KEY (`uid`,`nid`),
KEY `nid` (`nid`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COMMENT='A record of which users have read which nodes.'
1 row in set (0.03 sec)
mysql [d8site]> show create table node\G
*************************** 1. row ***************************
Table: node
Create Table: CREATE TABLE `node` (
`nid` int(10) unsigned NOT NULL AUTO_INCREMENT,
`vid` int(10) unsigned DEFAULT NULL,
`type` varchar(32) CHARACTER SET ascii NOT NULL COMMENT 'The ID of the target entity.',
`uuid` varchar(128) CHARACTER SET ascii NOT NULL,
`langcode` varchar(12) NOT NULL,
PRIMARY KEY (`nid`),
UNIQUE KEY `node_field__uuid__value` (`uuid`),
UNIQUE KEY `node__vid` (`vid`),
KEY `node_field__type__target_id` (`type`)
) ENGINE=InnoDB AUTO_INCREMENT=1066 DEFAULT CHARSET=utf8 COMMENT='The base table for node entities.'
1 row in set (0.00 sec)
~~~~~~~~~~~~~~~~~~~~~~~~~~~
mysql [d7site]> show create table history\G
*************************** 1. row ***************************
Table: history
Create Table: CREATE TABLE `history` (
`uid` int(11) NOT NULL DEFAULT '0' COMMENT 'The users.uid that read the node nid.',
`nid` int(11) NOT NULL DEFAULT '0' COMMENT 'The node.nid that was read.',
`timestamp` int(11) NOT NULL DEFAULT '0' COMMENT 'The Unix timestamp at which the read occurred.',
PRIMARY KEY (`uid`,`nid`),
KEY `nid` (`nid`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COMMENT='A record of which users have read which nodes.'
1 row in set (0.00 sec)
mysql [d7site]> show create table node\G
*************************** 1. row ***************************
Table: node
Create Table: CREATE TABLE `node` (
`nid` int(10) unsigned NOT NULL AUTO_INCREMENT COMMENT 'The primary identifier for a node.',
`vid` int(10) unsigned DEFAULT NULL COMMENT 'The current node_revision.vid version identifier.',
`type` varchar(32) NOT NULL DEFAULT '' COMMENT 'The node_type.type of this node.',
`language` varchar(12) NOT NULL DEFAULT '' COMMENT 'The languages.language of this node.',
`title` varchar(255) NOT NULL DEFAULT '' COMMENT 'The title of this node, always treated as non-markup plain text.',
`uid` int(11) NOT NULL DEFAULT '0' COMMENT 'The users.uid that owns this node; initially, this is the user that created it.',
`status` int(11) NOT NULL DEFAULT '1' COMMENT 'Boolean indicating whether the node is published (visible to non-administrators).',
`created` int(11) NOT NULL DEFAULT '0' COMMENT 'The Unix timestamp when the node was created.',
`changed` int(11) NOT NULL DEFAULT '0' COMMENT 'The Unix timestamp when the node was most recently saved.',
`comment` int(11) NOT NULL DEFAULT '0' COMMENT 'Whether comments are allowed on this node: 0 = no, 1 = closed (read only), 2 = open (read/write).',
`promote` int(11) NOT NULL DEFAULT '0' COMMENT 'Boolean indicating whether the node should be displayed on the front page.',
`sticky` int(11) NOT NULL DEFAULT '0' COMMENT 'Boolean indicating whether the node should be displayed at the top of lists in which it appears.',
`tnid` int(10) unsigned NOT NULL DEFAULT '0' COMMENT 'The translation set id for this node, which equals the node id of the source post in each set.',
`translate` int(11) NOT NULL DEFAULT '0' COMMENT 'A boolean indicating whether this translation page needs to be updated.',
PRIMARY KEY (`nid`),
UNIQUE KEY `vid` (`vid`),
KEY `node_changed` (`changed`),
KEY `node_created` (`created`),
KEY `node_frontpage` (`promote`,`status`,`sticky`,`created`),
KEY `node_status_type` (`status`,`type`,`nid`),
KEY `node_title_type` (`title`,`type`(4)),
KEY `node_type` (`type`(4)),
KEY `uid` (`uid`),
KEY `tnid` (`tnid`),
KEY `translate` (`translate`),
KEY `language` (`language`)
) ENGINE=InnoDB AUTO_INCREMENT=2442 DEFAULT CHARSET=utf8 COMMENT='The base table for nodes.'
Comment | File | Size | Author |
---|---|---|---|
#21 | d7-value_range_of_node_nid-2633334-21.patch | 983 bytes | orbmantell |
#15 | value_range_of_node_nid-2633334-15.patch | 2.17 KB | orbmantell |
Comments
Comment #2
e._s CreditAttribution: e._s commentedComment #3
e._s CreditAttribution: e._s commentedComment #4
e._s CreditAttribution: e._s at Acquia commented.
Comment #5
japerryBumping up to 8.x, will need a install patch for both 8.x and 7.x
Comment #6
e._s CreditAttribution: e._s at Acquia commentedGot help with a patch to d8. Attaching.
Comment #7
orbmantell CreditAttribution: orbmantell as a volunteer commentedI have tested #6 both as an update and as part of a new install. As best I can tell, the patch functions as intended. All indexes and unique keys remain after the patch/update is run, and the history.nid field is altered to be unsigned.
Comment #8
catchThis shouldn't use the values from hook_schema() since those might change again after the update runs, at which point it would update with the later schema, which might not be compatible with the table (say it added another column) or could conflict with something else.
So the history index should be specified directly here instead.
Also this needs a new update defgroup, and the update should be named '8101' instead of '8001'.
This is potential data loss, so I'd be tempted to put it into 8.0.x too, but also prefer not having update functions in patch releases if we can avoid it, so leaving against 8.1.x for now.
Comment #9
orbmantell CreditAttribution: orbmantell as a volunteer commentedHere is a new patch with the
drupal_get_module_schema()
call replaced with a hardcoded schema array and the update hook number changed to 8101.I am confused as to the comment about this patch needing a new update defgroup, as I can't find any other updates with a defgroup (so I omitted adding one to this patch).
Comment #10
orbmantell CreditAttribution: orbmantell as a volunteer commentedBackport patch for D7
Comment #12
orbmantell CreditAttribution: orbmantell as a volunteer commentedUpdating my patch so it only calls
Database::getConnection()->schema()
once.Additionally, I am adding a D7 backport patch here.
Comment #13
TravisCarden CreditAttribution: TravisCarden at Acquia commentedThe patch in #12 looks good from correctness and coding standards perspectives. Since we don't have any automated tests for the fix (and given the nature of it, they're probably neither necessary nor desirable), if we can just have somebody other than the patch author confirm that it actually works (the issue reporter would be nice) we can probably put this back to RTBC once the tests pass and given one thing: @catch, did you mean that this patch should include the update
defgroup
, or is that something that gets added at another time? This is the first I've seen of those.Comment #14
catch@TravisCarden yes I meant the update @defgroup. We have very few updates in 8.x compared to 7.x, so it's not come up much, but we need to start doing that now. There are lots of examples in 7.x's system.install
Comment #15
orbmantell CreditAttribution: orbmantell as a volunteer commentedAdding a defgroup to the patch in #12, and re-uploading the backport patch to keep the two patches together.
Comment #16
e._s CreditAttribution: e._s at Acquia commentedtested and works for me
Comment #17
catchThis looks good to me, but sending it for testing on all environments before commit.
Comment #18
catchCommitted/pushed to 8.1.x, thanks!
Comment #20
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThe Drupal 7 patch in #15 looks very good to me; the only issue I saw is that the new update function should be inside the "updates-7.x-extra" doc group (rather than outside it).
Comment #21
orbmantell CreditAttribution: orbmantell as a volunteer commentedRe-rolling the D7 patch to move the update hook into the "updates-7.x-extra" doc group.
Comment #23
orbmantell CreditAttribution: orbmantell as a volunteer commentedThe patch in #21 failed testing with an error
I don't know enough about the automated testing configuration (and therefore if the "node.install" file referenced on the test server is correct or not), but the patch references node.install based on the root of the Drupal install (/modules/node/node.install). I'm confused why it would not apply correctly during the test as I have verified the patch applies cleanly on a local setup.
Comment #24
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedYeah, that looks like some kind of weird testbot error. I'm going to trigger a retest now and see if it does better the second time.
Comment #26
orbmantell CreditAttribution: orbmantell as a volunteer commentedNo clue why the test in #21 failed twice before... but a re-test today has it passing.
Comment #27
e._s CreditAttribution: e._s at Acquia commentedThis does exactly what I want- thanks!
Comment #28
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC + 1, marking for commit
Comment #29
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted to 7.x - thanks!