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.'
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

e._s created an issue. See original summary.

e._s’s picture

Issue summary: View changes
e._s’s picture

Issue summary: View changes
e._s’s picture

japerry’s picture

Version: 7.x-dev » 8.1.x-dev

Bumping up to 8.x, will need a install patch for both 8.x and 7.x

e._s’s picture

Component: node system » history.module
Status: Active » Needs review
FileSize
1.33 KB

Got help with a patch to d8. Attaching.

orbmantell’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

catch’s picture

Title: value range of node.nid does not match history.nid » Unsigned int vs. int mismatch between node.nid and history.nid
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs work
Issue tags: +D8 upgrade path, +Needs backport to D7
+++ b/core/modules/history/history.install
@@ -39,3 +42,21 @@ function history_schema() {
+  $spec = drupal_get_module_schema('history', 'history');

This 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.

orbmantell’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Here 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).

orbmantell’s picture

Backport patch for D7

Status: Needs review » Needs work

The last submitted patch, 10: d7-value_range_of_node_nid-2633334-10.patch, failed testing.

orbmantell’s picture

Updating my patch so it only calls Database::getConnection()->schema() once.

Additionally, I am adding a D7 backport patch here.

TravisCarden’s picture

The 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.

catch’s picture

@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

orbmantell’s picture

e._s’s picture

Status: Needs review » Reviewed & tested by the community

tested and works for me

catch’s picture

This looks good to me, but sending it for testing on all environments before commit.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x, thanks!

  • catch committed 82ec93d on 8.1.x
    Issue #2633334 by orbmantell, e._s: Unsigned int vs. int mismatch...
David_Rothstein’s picture

Version: 8.1.x-dev » 7.x-dev
Status: Fixed » Needs work
Issue tags: +Needs backport to D7

The 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).

orbmantell’s picture

Status: Needs work » Needs review
FileSize
983 bytes

Re-rolling the D7 patch to move the update hook into the "updates-7.x-extra" doc group.

Status: Needs review » Needs work

The last submitted patch, 21: d7-value_range_of_node_nid-2633334-21.patch, failed testing.

orbmantell’s picture

The patch in #21 failed testing with an error

error: sites/all/modules/drupal/modules/node/node.install: No such file or directory
Patch Failed to apply

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.

David_Rothstein’s picture

Status: Needs work » Needs review

Yeah, 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.

Status: Needs review » Needs work

The last submitted patch, 21: d7-value_range_of_node_nid-2633334-21.patch, failed testing.

orbmantell’s picture

Status: Needs work » Needs review

No clue why the test in #21 failed twice before... but a re-test today has it passing.

e._s’s picture

Status: Needs review » Reviewed & tested by the community

This does exactly what I want- thanks!

Fabianx’s picture

Issue tags: +Pending Drupal 7 commit

RTBC + 1, marking for commit

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit +7.50 release notes

Committed to 7.x - thanks!

  • David_Rothstein committed 0dcc87a on 7.x
    Issue #2633334 by orbmantell, e._s, catch, TravisCarden: Unsigned int vs...

Status: Fixed » Closed (fixed)

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