Database primary keys can exceed maximum integer storeable (has actually occurred, in watchdog)

This could technically happen in any table, but watchdog and cache tables are most susceptible.

cache tables don't have autoincrement primary keys.

Noted!

Huh, this is tricky. According to the docs the counter can't be reset to a value smaller than the max.
So we have to either truncate the table or renumber it (e.g. delete and recreate the ID column, plus some voodoo to keep order).

Do we really, really need autoincrement on the watchdog table? What about dropping the column?

How would we identify a single log message then and e.g. view it in the site?

I'm not so sure about the severity here. Yes, it is ugly, but it's also pretty easy to fix, just the clear log confirmation form/action does a TRUNCATE query, that should afaik reset the ID's, so if you run that when it happens then you lost your recent logs, but everything should work again then.

One strange thing is that the wid is apparently signed, so we're wasting 50% of the space we could have.

But even then, it's still quite a big number, and you have a site where this happens, then you should probably fix that so many notices are logged, because both having so many php notices/warnings and logging them into the database is a serious performance issue. Or as a quickfix, use syslog instead of the database.

While personally never saw a big issue with it in our sites, it has been the recommendation for a long time to disable dblog on a high-performance or even any production site and use something else.

So IMHO, I don't think this even qualifies as major yet alone a critical?

How would we identify a single log message then and e.g. view it in the site?

We might have to use uuid instead of autoincrement ID. Don't love that really either.

But even then, it's still quite a big number, and you have a site where this happens, then you should probably fix that so many notices are logged, because both having so many php notices/warnings and logging them into the database is a serious performance issue.

So, while this is true we're talking about sites that are running for multiple years.

> So, while this is true we're talking about sites that are running for multiple years.

Sure. But unless I made a mistake, if a site logs 10 log entries per second then that's still ~7 years until you reach that limit. Twice as much if we change it to an unsigned integer. At that point, I'm pretty sure that it would be a serious performance issue long before this happens, so you have bigger issues?

If you keep 100k log records, then you can keep your log records for ~3h, at that point, why have dblog enabled at all?

I do not think that truncating the whole watchdog table is a good idea.
You want to throw out the old messages, but keep the newer ones.
The problem with that is that the autoincrement counter cannot be reset.
Maybe it would be a good idea to change the autoincrement key to a key that just generates a unique value, uniqid().
Then there will be no more maximum value that can be reached.

Here's a SQL fiddle how this can work IF we want this.
http://sqlfiddle.com/#!9/19f165/1

CREATE TABLE IF NOT EXISTS `watchdog` (
  `id` int(6) unsigned NOT NULL auto_increment,
  `text` varchar(200) NOT NULL,
  PRIMARY KEY (`id`)
) DEFAULT CHARSET=utf8;
INSERT INTO `watchdog` (`text`) VALUES
  ('One'),
  ('Two'),
  ('Three'),
  ('Four');
delete from `watchdog` where id in (1,2);
select min(id) from watchdog; /* 3 */
update watchdog set id=id+1-3;
ALTER TABLE watchdog AUTO_INCREMENT = 1;
INSERT INTO `watchdog` (`text`) VALUES
  ('Five');

That said, i'd rather make the WID a bigint.

The one problem with uniqid() is that autoincrement currently guarantees that watchdog messages can be ordered chronologically - the timestamp doesn't since obviously things can be logged in the same millisecond.

The one problem with uniqid() is that autoincrement currently guarantees that watchdog messages can be ordered chronologically - the timestamp doesn't since obviously things can be logged in the same millisecond.

Gets then go for the bigint solution.

Attached is a patch for suggestions from geek-merlin and daffie.

'wid' => [
'type' => 'serial',
'not null' => TRUE,
+ 'size' => 'big',
'description' => 'Primary Key: Unique watchdog event ID.',
],

Setting to needs review to let the tests run. Update tests are going to fail, this needs a corresponding update function.

The patch needs an upgrade path, so that the size of the wid column of the watchdog table in existing sites is changed to big.

And we also need an explicit test then, because as a non-breaking database structure change, nothing picks this up as a problem right now.

That was really friggin' tricky to write a test for, but I tried. I'm not sure this is exactly what we want, because even with the patch in #18, the test still fails for me. So either the test I wrote is flawed (very likely), or the bigint solution won't work. Thoughts?

So #24 is the test only which fails as it should.
(Yay! we can use anon classes now, was not aware of this!)

If you post test-plus-fix patch, maybe the failure messages give someone a hint.

From a quick glance, it looks to me that the anonymous class does not get $decorated set (but maybe that's my lack of deep phpunit groking).

What I was asking for is an upgrade path test, which should be trivial :). That's certainly a very creative test, but yes of course, the current patch isn't going to fix anything, it's just going to make it happen later.

I still think that we don't have to do more to fix it and also don't think this is critical :) An alternative option would be to do a requirements check that checks if we're getting close to the limit and recommends to empty the table *before* it happens.

If we start to replace autoincrement (ish) fields because of this, where does it stop? it's probably the most likely table to happen, but it's not impossible for others either.

Here's a patch combining the change in #18 with the test from #24. Let's see what it does. Also tagging for an upgrade path test per #27.

Okay, here's an attempt at a tested update path.

This should be probably be signed off on by someone who's more knowledgeable than me when it comes to databases, since it relies on PHP_INT_SIZE and assumes that a "normal" integer column will not contain a 64-bit value (which is likely incorrect on SQLite, for a start).

For the record, I agree with this:

the current patch isn't going to fix anything, it's just going to make it happen later.

I don't know what an alternative would be, but making the column support big integers is just kicking the can down the road, and may very well fail at a certain scale.

Why, exactly, can't we switch to using UUIDs?

Thank you for letting me chime in; I hope I don't muddy the waters further.

Cleanest solution would be to log to watchdog using microtime()
and not use an autoincrement primary key (wid).

We would need to change to pruning function in dblog_cron so that it prunes
based on a time span rather than a number of records, but that might actually
be conceptually clearer for users.

The issue of a INT database primary key reaching maximum integer is possible,
but rare.

2^31 log entries at 1k per entry is 2 TB of data -- most hosts would stop you
long before you reached that.

But if you were hosting on AWS, and had not set a database size limit, and
if you were the victim of DDOS attack, you could possibly reach autoincrement max.

Alternate approach:

In DbLog.php, catch the database error and turn off the dblog module

\Drupal::service('module_installer')->uninstall(['dblog']);

> 2^31 log entries at 1k per entry is 2 TB of data -- most hosts would stop you long before you reached that.
> But if you were hosting on AWS, and had not set a database size limit, and if you were the victim of DDOS attack, you could possibly reach autoincrement max.

Deleting records doesn't reset the ID, so you don't actually have 2 TB of data, you could reach that with the limit set to 1000.

Why, exactly, can't we switch to using UUIDs?

@phenaproxima the autoincrement gives us a definite sequence to sort by, for debugging we need to the sort to be more accurate than per-second.

As @mindbet mentions in #33 another option might be to log with microtime() but we'd need to make that change at the same time and update any sorts.

Here is an updated patch to convert the watchdog primary key (wid) to bigint.

Comments on approach:

It is easily possible to add 25 log events per second to watchdog.
I was able to do this in local dev using ab.

At 25 logs/sec, the INT space (2^31) will be full in 2.72 years.
The BIGINT space (2^63) will last 11+ billion years.

The microtime suggestion, on further consideration, is not a good route.

microtime produces either a float or a string

The float is limited precision.

The string looks something like this:

0.88814800 1593953860

That string would need to be processed for every log entry.

https://www.php.net/manual/en/function.microtime.php

I tested the SQL command

ALTER TABLE `watchdog` CHANGE `wid` `wid` BIGINT NOT NULL AUTO_INCREMENT COMMENT 'Primary Key: Unique watchdog event ID.'

on local, on 100,000 records, and it took about 2 seconds.

There is a lot of discussion of the INT to BIGINT problem online, for example see:

https://sqlperformance.com/2016/01/sql-indexes/widening-identity-column-1

Corrected patch from #36.

Revision of 3081144-30-PASS.patch to fix:

1075 Incorrect table definition

As of the suggestion to use microtime: It sounds appealing on first hear.
But with an autoincrement ID we get a sort of integrity constraint that we know when a log entry is deleted. We'd lose that.

adding related issue:
https://www.drupal.org/project/drupal/issues/3024410

Here is a revised patch.

I used some direct SQL statements to work around issues where it did not appear that $schema->changeField was making all of the necessary changes.

There is no db_type specific code for sqlite because sqlite appears to handle 8-byte integers out of the box, so no update needed.

The test in #30 still needs work IMHO — we need to check that watchdog.wid can accept a value greater than 2**31, but also need to check that subsequent writes to watchdog are successful.

Code syntax fixes
Mostly to get Postgres test working.

Trying another way to get Postgres changefield to work.

Postgres changes are now made using the schema api.

In order to do this, I needed to make changes to the pgsql driver, in part based on code in this issue:

https://www.drupal.org/project/drupal/issues/3028706

Patch 46 fixes some things in patch 45.

This is removed:

function updateSequenceOwnership

because it belongs to issue 3028706, not this issue.

Also there was parameter commented out for testing that is now restored:

diff -u b/core/modules/dblog/dblog.install b/core/modules/dblog/dblog.install
--- b/core/modules/dblog/dblog.install
+++ b/core/modules/dblog/dblog.install
@@ -17,7 +17,7 @@
'wid' => [
'type' => 'serial',
'not null' => TRUE,
-# 'size' => 'big',
+ 'size' => 'big',

1. +++ b/core/modules/dblog/tests/src/Functional/UpdatePathTest.php
   @@ -0,0 +1,76 @@
   +    if (PHP_INT_SIZE !== 8) {
   

   Can we change !== to <.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -958,7 +961,7 @@ EOD;
   -      $this->connection->query("CREATE SEQUENCE " . $seq);
   +      $this->connection->query("CREATE SEQUENCE IF NOT EXISTS " . $seq . " OWNED BY {" . $table . "}." . $field_new);
   

   I think that this change is out of scope for this issue.

3. +++ b/core/modules/dblog/dblog.install
   @@ -97,3 +100,46 @@ function dblog_schema() {
   +  if ($db_type == 'pgsql') {
   ...
   +  if ($db_type == 'mysql') {
   

   Can we just have one script for all databases and then add exceptions for specific databases.

4. +++ b/core/modules/dblog/dblog.install
   @@ -97,3 +100,46 @@ function dblog_schema() {
   +    \Drupal::messenger()->addMessage('pgsql');
   ...
   +    \Drupal::messenger()->addMessage('Added sequence');
   ...
   +    \Drupal::messenger()->addMessage('mysql');
   

   These messages are not necessary.

5. +++ b/core/modules/dblog/tests/src/Functional/UpdatePathTest.php
   @@ -0,0 +1,76 @@
   +    $this->runUpdates();
   

   Can we check before running the update script what the database field value is for "wid". Regular serial field and not big.

6. +++ b/core/modules/dblog/tests/src/Functional/UpdatePathTest.php
   @@ -0,0 +1,76 @@
   +    $insert = Database::getConnection()->insert('watchdog');
   

   I do not this this is necessary. Just query the database for the field values of the field "wid". It should be big now.

Addressed point => 1,2,4.
WIll be working on the remaining 3.

Postponing this issue on #3191391: Schema::changeField() has bug when changing regular serial field to big serial field.

Reset issue status.

Add template and add postponed issue to remaining tasks.

Adding credit from #3024410: Watchdog wid column should be unsigned which I closed as a duplicate

#3191391: Schema::changeField() has bug when changing regular serial field to big serial field was merged for D10 and I just provided the backport for D9.5 I think this is now unblocked.

Upgraded to work with Drupal 10.1.x. The lines from patch #49 regarding core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php now seems included in another file core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php.

I think I also addressed comment .3 from #48

Thanks for this input @dagmar, I think we could make this .install function even shorter using some inspiration from issue #3191391.

Let's see if this breaks the tests or not.

I updated the issue summary to describe the proposed solution.

+++ b/core/modules/dblog/tests/src/Functional/UpdatePathTest.php
@@ -0,0 +1,79 @@
+    $insert = Database::getConnection()->insert('watchdog');
+
+    $connection = $this->prophesize(Connection::class);
+    $connection->getTarget()->willReturn(DbLog::DEDICATED_DBLOG_CONNECTION_TARGET);
+    $connection->insert('watchdog')->willReturn(new class ($insert) {
+
+      /**
+       * The decorated query object.
+       *
+       * @var \Drupal\Core\Database\Query\Insert
+       */
+      private $decorated;
+
+      /**
+       * Constructor for this anonymous class.
+       *
+       * @param \Drupal\Core\Database\Query\Insert $decorated
+       *   The decorated query object.
+       */
+      public function __construct(Insert $decorated) {
+        $this->decorated = $decorated;
+      }
+
+      /**
+       * Overrides \Drupal\Core\Database\Query\Insert::fields().
+       *
+       * Purposefully sets the 'wid' column of the query to a value which will
+       * overflow the 32-bit integer limit.
+       */
+      public function fields(array $fields, array $values = []) {
+        $fields['wid'] = PHP_INT_MAX;
+        return $this->decorated->fields($fields, $values);
+      }
+
+    });
+    $this->container->set('database', $connection->reveal());
+    $this->generateLogEntries(1);

Can we replace this part with:
- insert row into table with a value for wid that is bigger than the regular maximum integer value.
- test that the row exists in the table with the correct value for the wid field
- insert another row without a value for wid. With the auto-increment functionality it should get the previous wid value plus 1.
- test that the new row exists in the table with the correct value for the wid field.

added updated patch and addressed #71

1. +++ b/core/modules/dblog/tests/src/Functional/UpdatePathTest.php
   @@ -0,0 +1,78 @@
   +    $connection = $this->prophesize(Connection::class);
   

   We do not need to prophesize the database connection. Just use the regular one

2. +++ b/core/modules/dblog/tests/src/Functional/UpdatePathTest.php
   @@ -0,0 +1,78 @@
   +      $fields['wid'] = PHP_INT_MAX;
   

   Can we change the value of PHP_MAX_INT with its value times 1000. That will be 2147483647000.

All we need to do is to test that we can insert a value for the wid field that is bigger than 2147483647 and that for values bigger than that the auto increment functionality works. No prophesize functionality needed.

Addressed #73

1. +++ b/core/modules/dblog/tests/src/Functional/UpdatePathTest.php
   @@ -0,0 +1,63 @@
   +    $insert->fields(['wid' => PHP_INT_MAX]);
   ...
   +      ->condition('wid', PHP_INT_MAX)
   ...
   +      ->condition('wid', PHP_INT_MAX + 1)
   

   Can you replace PHP_INT_MAX with 2147483647000.

2. +++ b/core/modules/dblog/tests/src/Functional/UpdatePathTest.php
   @@ -0,0 +1,63 @@
   +    $insert = Database::getConnection()->insert('watchdog');
   ...
   +    $insert = Database::getConnection()->insert('watchdog');
   ...
   +    $result = Database::getConnection()->select('watchdog')
   ...
   +    $result = Database::getConnection()->select('watchdog')
   

   Can we use the variable $connection. Please add: $connection = Database::getConnection();

Addressed #75 comment.
Thanks

@Akram Khan and @pratik: You are supposed to run the test you are working on your local machine. To make sure that it passes. Now it is not.

I've adjusted the patch so the test don't fail. The dblog table has a lot of not-null fields, so we need to fill these to insert the log.

I've also removed the use cases for FakeLogEntries and ProphecyTrait, we still pass the test and we don't need to alter the baseline anymore.

All code changes look good to me.
I have updated the IS.
For me it is RTBC.

Committed e9a82dd and pushed to HEAD. Thanks!