Update hook_schema for Y2038

This looks great, my only concern is that if we add an update test for each module it's going to increase the time it takes to run our test suite dramatically

Is there a way we can have one test, and each module adds its fixtures and asserts to that?

+++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdate.php
@@ -0,0 +1,74 @@
+   * Tests update 9301.

I think this comment can probably be more generic now its testing multiple updates

Other than that, I think this is looking really good.

Should we postpone the other issues on getting this in so we can then build on top of this?

I think we should probably have 2 two issues for 2038 issue - one for \Drupal\Core\Field\Plugin\Field\FieldType\TimestampItem (ie. #2885413: Timestamp field items are affected by 2038 bug) and one for the rest which are changing hook_schema or pseudo hook_schema (i.e BatchStorage::schemaDefinition()) - I think per module here is probably the wrong scope and will introduce unnecessary inconsistency.

1. +++ b/core/modules/forum/forum.install
   @@ -175,3 +175,30 @@ function forum_schema() {
   +function forum_update_9301(&$sandbox = NULL) {
   

   Need to change forum_schema() too.

2. +++ b/core/modules/system/system.install
   @@ -1504,3 +1504,20 @@ function _system_advisories_requirements(array &$requirements): void {
   +function system_update_9301(&$sandbox = NULL) {
   

   Need to change system_schema() too.

+++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
@@ -0,0 +1,124 @@
+    $tables = $connection->schema()->findTables('migrate_map_%');
...
+    $tables = $connection->schema()->findTables('cache_%');

should we assert that both of these are not empty, so we know we have test coverage for at least one of these dynamic tables of each

I think the only task left here is to also ensure that we got all of them (manually I guess?)

1. +++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
   @@ -0,0 +1,125 @@
   +      DRUPAL_ROOT . '/core/modules/system/tests/fixtures/update/drupal-9.1.0.update.standard.php.gz',
   

   I am not saying that adding a new drupal 9.1 update standard is wrong, only could you explain why it is added and what is in it.

2. +++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
   @@ -0,0 +1,125 @@
   +        $result = $this->connection->query("SELECT data_type FROM information_schema.columns WHERE table_schema = '$this->databaseName' and table_name='$table_name' and column_name='$column_name';")
   +          ->fetchField();
   

   I think this only works for MySQL databases.

3. I think we need a CR for module maintainers how to do the same for their module (contrib and custom). This should also include code examples.

+++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
@@ -0,0 +1,125 @@
+        $result = $this->connection->query("SELECT data_type FROM information_schema.columns WHERE table_schema = '$this->databaseName' and table_name='$table_name' and column_name='$column_name';")
+          ->fetchField();

This does not work for PostgreSQL and SQLite. This will get fixed in #301038: Add a cross-compatible database schema introspection API. Therefor postponing this issue.

Talked to @catch on Slack about this and his suggestion was to not postpone this issue and specific code for each of the by core supported databases, because this is only in a test.

Updated the patch for the failing Y2038TimestampUpdateTest. SQLite does not to update as it is not affected. Therefor Y2038TimestampUpdateTest can be skipped by SQLite. As SQLite does not have information_schema to query. It has something else, only that is not necessary for this issue.

@quietone: Thank you for explaining why the new fixture was added.

I have created a CR.

All my points from comment #18 are addressed.

TBH I have mostly nitpicks:

1. +++ b/core/modules/comment/comment.install
   @@ -116,3 +117,20 @@ function comment_schema() {
   +  if ($connection->schema()->tableExists('comment_entity_statistics') && $connection->databaseType() != 'sqlite') {
   

   I read and understood why this change has been made, but exactly because there is no difference between sqlite's integer sizes I would remove this checks on all schema updates. Removing the checks will help with code readability and - as already said - won't make any difference.

2. +++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
   @@ -0,0 +1,140 @@
   +      // enabled modules, forum, language, locale, statistics and tracker
   

   Typo enabled modules, -> enabled modules:

3. +++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
   @@ -0,0 +1,140 @@
   +      // modules enabled.
   

   We can get rid of this line, and just finish the one before with a full stop?

4. +++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
   @@ -0,0 +1,140 @@
   +    public function assertTimestampFields($expected_values) { ... }
   

   I would split this method in two, a collectTimestampFieldsFromDatabase(), filling in $this->table property, and leaving in assertTimestampFields() only the assertion bit.

For #28.1: SQLite does not need to change, because SQLite uses a more general dynamic type system. The size of the value determines the size that is used on disk. For more info: https://www.sqlite.org/datatype3.html.
We can remove the exceptions for SQLite, only the problem is that we need to test it and SQLite does not have "information_schema.columns" table/view. And yes it makes the patch a bit more cleaner, only the testing will be more difficult. I can live with how we are doing it now.

Back to needs work for:

+++ b/core/modules/comment/comment.install
@@ -116,3 +117,20 @@ function comment_schema() {
+function comment_update_9301(&$sandbox = NULL) {

This is not going in D9.3, therefor the function name should be: "comment_update_9401()". The same for all the other function names with "_9301" in it. I have updated the CR.

only the problem is that we need to test it and SQLite does not have "information_schema.columns" table/view

Hey @daffie, I'm with you with that. In fact my suggestion is to remove the check only on hook_schema() & Co., but keep it in that test.

Then yes, we should test SQLite as well. Why don't we change/extend the test so to INSERT a big int, we query it and we make sure has been stored correctly? Thoughts?

I would like to add SQLite support too. Only this change does nothing for SQLite. For SQLite there is just one integer type. Therefor I think that this is also untestable for SQLite. Before the running the update all time fields are integers and after the update this is still the same. Keeping the check in hook_update_N() is for me the slightly better option, because the change does no do anything for SQLite. I shall leave it to @quietone to decide to keep it or to remove it as she is the one who is making the patch.

Thanks @daffie for the feedback and @quietone for the changes.

My worry about keeping the conditional check in hook_schema() & Co. is inconsistency in Drupal's schema when using different DB types. AKA exploring the schema metadata for MySQL/PgSQL will say Field X is a int:big while for SQLite will say Field X is int:normal. But I do understand for SQLite maps to the same storage is eventually the same, so no more objection.
Also I don't seem to be able to find a way to fetch the database - Drupal API - schema details for a field/table... so I'm worrying for nothing? :D :D

Moving this to RTBC. The failure on #32 is a glitch on FunctionalJavascript testing.

1. +++ b/core/modules/system/system.install
   @@ -1651,3 +1653,96 @@ function _system_check_array_table_prefixes() {
   +  $cache_tables = $schema->findTables('cache%');
   

   Why not cache_*

2. +++ b/core/modules/system/tests/fixtures/update/Y2038-timestamp.php
   --- /dev/null
   +++ b/core/modules/system/tests/fixtures/update/drupal-9.1.0.update.standard.php.gz
   

   I don't understand we we're not using drupal-9.0.0.filled.standard.php.gz. If we use that dump then we only have to create the migrate_map_d7_file in core/modules/system/tests/fixtures/update/Y2038-timestamp.php. I think this is way better than maintaining another dump. This dump has batch and flood tables already and also has all the necessary modules enabled.

   Maintaining another db dump is not a free activity.

3. +++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
   @@ -0,0 +1,145 @@
   +    $tables = $connection->schema()->findTables('cache_%');
   +    $tables = array_diff($tables, ['cachetags']);
   

   cachetags will not be found when looking for cache_

+++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
@@ -0,0 +1,145 @@
+      $this->markTestSkipped();

We should have a helpful message here. For example

$this->markTestSkipped("This test does not support the SQLite database driver.");

We're also going to need a 10.0.x version of the patch as well.

#35.1 and #35.3 , see #3269091: Undocumented behaviour for Schema::findTables() when an underscore is used

This patch addresses feedback from #35.2 deleting the new fixture and using 9.3.0.filled.
Also since #35.1 and #35.3 can't be addressed due #3269091: Undocumented behaviour for Schema::findTables() when an underscore is used I made the code consistent, ready for when that issue is fixed and adding TODOs so we remove the array_diff($tables, ['cachetags']) lines when the time comes.

I'm not a big fan of TODOs, but since this one is critical and the related issue is not I'm leaning towards allowing the TODO.

Still needs re-roll for 10.0.x

This is an as-is reroll of #39, but for 10.0.x. So this include the CS issues #39-9.4.x has got as well.

Leaving Needs Work to fix the CS issues.

This is #39 (and #40) patches with he IS fixed.

@gambry nice work on filing #3269091: Undocumented behaviour for Schema::findTables() when an underscore is used

+++ b/core/modules/system/system.install
@@ -1651,3 +1653,98 @@ function _system_check_array_table_prefixes() {
+  // @todo Remove/review the line below when following issue is fixed
+  //   https://www.drupal.org/project/drupal/issues/3269091
+  $cache_tables = array_diff($cache_tables, ['cachetags']);

+++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
@@ -0,0 +1,147 @@
+    $tables = $connection->schema()->findTables('cache_%');
+    // @todo Remove/review the line below when following issue is fixed
+    //   https://www.drupal.org/project/drupal/issues/3269091
+    $tables = array_diff($tables, ['cachetags']);

I think we need to remove all entries not starting with cache_ - just in case a contrib or custom module has added something similar to cachetags. Something like

$tables = array_filter($tables, function ($table) { return str_starts_with($table, 'cache_');});

I think the solution to #3269091: Undocumented behaviour for Schema::findTables() when an underscore is used is to improve the documentation.

Made changes as per comment #42.

There are CS issues to be fixed. Also I noticed change on #36 still needs to be addressed.

CS issues fixed & addressed #36

Amazing, I can see feedback from #42 ad #36 has been addressed, as well as CS issues from #43.

The only missing bit is rolling the same fixes on #41-10.0.x patch. So setting needs work for this reason.

Also hiding some files not needed anymore.

Added a patch for Drupal 10 with fixes of comment #44.

I reviewed the patch thoroughly and all looks great!

However... one more thing:

+++ b/core/modules/system/system.install
@@ -1569,3 +1571,100 @@ function _system_advisories_requirements(array &$requirements): void {
+  // @todo Remove/review the line below when following issue is fixed
+  //   https://www.drupal.org/project/drupal/issues/3269091

+++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
@@ -0,0 +1,149 @@
+    // @todo Remove/review the line below when following issue is fixed
+    //   https://www.drupal.org/project/drupal/issues/3269091

I believe we can now remove these TODOs.

Addressed comment #49.

Nice work everyone. I presume this can be RTBC now.

The only thing I'm not be able to assess is how long the update(s) will take for big datasets. I know I tried this somewhere else, but I can't find where anymore. :(
The other face of the problem is: I don't think there is a lot we can do. We can't batch column size updates, so even though that will be slow - if it will be - we can't avoid it. But maybe we can advice better? So does the Change records need a line about it?

str_starts_with() is a really nice function but I don't think it can be used on Drupal 9.4, as it's a PHP 8 function. Unless the minimum PHP version is going to change?

Maybe instead, escapeLike() can be called to escape the underscore (assuming that's the problem here, I didn't look into how findTables() works).

re: #52 I might have lied, I see that core uses symfony/polyfill-php80 so str_starts_with() should be ok to use?

I tested the update on a small-ish MySQL database and it looked like most of the time was spent updating the cache tables (which had around 5000 rows), which had to be copied to temporary table while altering. Perhaps this update could be optimized by truncating the cache tables before altering, rather than the current situation where they are altered only to be truncated later?

We can't batch column size updates

Actually these probably need to be batched using $sandbox? Although, if the cache tables are truncated first, perhaps the alter statements are fast enough to not be batched.

Actually these probably need to be batched using $sandbox?

I don't think that will help. We can batch the $connection->schema()->changeField() command, but since the long part will be waiting for DBs to apply the change batching PHP side may be quite useless.

I tested the update on a small-ish MySQL database and it looked like most of the time was spent updating the cache tables

Can you define what kind of timeframe are we talking about? Seconds? Minutes?

Perhaps this update could be optimized by truncating the cache tables before altering

Possibly. I'm not sure what policies are around truncating cache within hooks_update(), but I personally don't see any downside.

Also since most high traffic instances may use memcache/redis for cache tables, do we need to anything around that?

@mfb if you can report timings around running the updates, I'll gather attention around the cache_ tables topics.

+++ b/core/modules/system/system.install
@@ -1569,3 +1571,98 @@ function _system_advisories_requirements(array &$requirements): void {
+  $cache_tables = $schema->findTables('cache_%');
+  $cache_tables = array_filter($cache_tables, function ($cache_tables) {
+    return str_starts_with($cache_tables, 'cache_');
+  });
+  if ($connection->databaseType() != 'sqlite') {
+    foreach (array_keys($cache_tables) as $table) {
...
+    }
+  }

We don't need to worry about memcache/redis. In this lines we look for any 'cache_' table in the database, and for each one we find we apply the changes. That means the actual memcache/redis table are untouched (and it's not core's problems to make sure they support 2038 dates).
However this code will run if cache_* tables exists in the database, so flushing the cache will not flush the unused db tables.

On my instance, the cache_page table size on disk was 168MB and changeField() took 13 seconds; cache_dynamic_page_cache was 116MB and changeField() took 9 seconds; i.e. 13 megabytes per second or 79 seconds per gigabyte.

We can batch the $connection->schema()->changeField() command, but since the long part will be waiting for DBs to apply the change batching PHP side may be quite useless.

Yes, I was imagining a hypothetical case of a hosting environment that has a request timeout and the site has at least a couple large tables to alter; if the table alter statements are setup as batch operation then there is much less concern with hitting the timeout. I've seen some long-running database migrations use hook_update_N($sandbox) in the past, but no idea what the criteria was for doing so - just wanted to flag this for review.

1. +++ b/core/modules/dblog/dblog.install
   @@ -97,3 +98,20 @@ function dblog_schema() {
   +    $connection->schema()->changeField('watchdog', 'timestamp', 'timestamp', $new);
   

   This is the change I would worry about the most, I have seen sites that never clear watchdog and have multi-gigabyte sized tables as a result.

2. +++ b/core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
   @@ -0,0 +1,147 @@
   +      // Start with a standard install of Drupal 9.1.0 with the following
   +      // enabled modules: forum, language, locale, statistics and tracker.
   +      DRUPAL_ROOT . '/core/modules/system/tests/fixtures/update/drupal-9.3.0.filled.standard.php.gz',
   

   9.1.0 -> 9.3.0 in the comment

This is the change I would worry about the most, I have seen sites that never clear watchdog and have multi-gigabyte sized tables as a result.

Well, this module has just one changeField() call in the update function, so there isn't much that can be done, right? But if there are multiple changeField() calls in an update function then batch operation could be used.

Discussed with @gambry in Slack. There is no guaranteed way of preventing timeout in some cases, as we just cannot know how large some tables might be, nor how slow some database operation might be as it is hardware dependent.

The change reminded me of the utf8 to utf8mb4 switch in Drupal 7 (see https://www.drupal.org/node/2754539) where we opted in new sites by default but existing sites were given instructions and a contrib module to help them upgrade. This is an alternative option here - albeit more complicated to implement and support - where in a similar fashion we opt in new sites to Y2038 compliance by default via a switch in settings.php, and allow other sites to upgrade in their own time.

However, timeouts should not cause data loss; the schema change will complete in the background eventually and once this is done a retry is a no-op so as long as this is documented then users who experience a timeout in update.php or drush updb should be able to safely retry until the operations are all completed.

Regarding the cache tables I don't see an issue with truncating those beforehand as database updates invariably end in a cache clear anyway, so it saves us time if we can wipe that first, but there is still the issue of tables that contain persistent data that might be very large.

Let's Needs work in order to address #58.1 and have a look if we need to worry about PostgreSQL error on #50.

The documentation mentioned on #60 can be included in the Change Record? Since the schema change is in multiple places, and there are other issues tackling Y2038 schema changes from other perspectives, having this in code in a single place may be problematic.

Adding the documentation depends on someone to try this update in a big dataset. I'll try myself, but I don't have a Drupal big db so I need to create one. If a gentle soul has got one that would save me time and bandwidth. 🙏🏻🙏🏻

Quick timings running the updates on a fairly large db. comment_entity_statistics has 56k rows. At least one migrate_map table with half a million rows.

No cache tables (agreed we could truncate those before running the update).

time drush updb -y
 --------- ----------- --------------- ---------------------------------- 
  Module    Update ID   Type            Description                       
 --------- ----------- --------------- ---------------------------------- 
  system    9401        hook_update_n   9401 - Remove the year 2038 date  
                                        limitation.                       
  comment   9401        hook_update_n   9401 - Remove the year 2038 date  
                                        limitation.                       
  dblog     9401        hook_update_n   9401 - Remove the year 2038 date  
                                        limitation.                       
  history   9401        hook_update_n   9401 - Remove the year 2038 date  
                                        limitation.                       
  migrate   9401        hook_update_n   9401 - Remove the year 2038 date  
                                        limitation.                       
 --------- ----------- --------------- ---------------------------------- 


 // Do you wish to run the specified pending updates?: yes.                   

>  [notice] Update started: system_update_9401
>  [notice] Update completed: system_update_9401
>  [notice] Update started: comment_update_9401
>  [notice] Update completed: comment_update_9401
>  [notice] Update started: dblog_update_9401
>  [notice] Update completed: dblog_update_9401
>  [notice] Update started: history_update_9401
>  [notice] Update completed: history_update_9401
>  [notice] Update started: migrate_update_9401
>  [notice] Update completed: migrate_update_9401
 [success] Finished performing updates.

real	0m16.834s
user	0m2.211s
sys	0m0.394s

That seems fine to me tbh.

1. +++ b/core/modules/migrate/migrate.install
   @@ -11,3 +11,24 @@
   +
   +/**
   + * Remove the year 2038 date limitation.
   + */
   +function migrate_update_9401(&$sandbox = NULL) {
   +  $connection = \Drupal::database();
   +  $tables = $connection->schema()->findTables('migrate_map_%');
   +  if (!empty($tables) && $connection->databaseType() != 'sqlite') {
   +    foreach ($tables as $table) {
   +      $new = [
   

   It would theoretically be possible to batch these, possibly not necessary given the timings.

2. +++ b/core/modules/system/system.install
   @@ -1569,3 +1571,98 @@ function _system_advisories_requirements(array &$requirements): void {
   +
   +  // Update cache tables.
   +  $cache_tables = $schema->findTables('cache_%');
   +  $cache_tables = array_filter($cache_tables, function ($cache_tables) {
   +    return str_starts_with($cache_tables, 'cache_');
   +  });
   +  if ($connection->databaseType() != 'sqlite') {
   +    foreach (array_keys($cache_tables) as $table) {
   +      $new = [
   +        'description' => 'A Unix timestamp indicating when the cache entry should expire, or ' . Cache::PERMANENT . ' for never.',
   

   Does this need to be concerned about the cache_tags table (which isn't a cache table as such).

   Also this system update could be split into multiple sequential updates without having to add any batch logic if we wanted.

Re #62.2 it's called cachetags and we're removing it from the list of tables to process because it does not have a date... it just has an invalidations counter. The fun thing is that it is returned by $schema->findTables('cache_%'); because of #3269091: Undocumented behaviour for Schema::findTables() when an underscore is used.

Documenting that sites can clear caches and database log to speed up the update should help here. How long the table alter takes mainly depends on size of the table not number of rows - a site with large comment_entity_statistics table could easily have larger cache tables, but they have the ability to clear them first.

@mfb I think it is reasonable for the system_update_9401() to truncate the cache tables prior to changing them. They are going to be truncated later anyway.

While working on a patch to address #58.2 and truncating the cache tables on system_update_9401() I noticed all cache tables have the created column using signed DECIMAL(14,3) for timestamp with microseconds.

On MySQL DECIMAL(14,3) allow dates up to 5138-11-16 09:46:39 (date('Y-m-d H:i:s', 99999999999);). Although smaller than BIGINT range, I'm confident we are safe.
I'll check on PostgreSQL just to be safe, but I'm almost sure the range will be pretty much the same.

Here we go. Patches for 10.0.x and 9.4.x addressing #58.2 and truncating the cache tables in system_update_9401().

On MySQL DECIMAL(14,3) allow dates up to 5138-11-16 09:46:39 (date('Y-m-d H:i:s', 99999999999);)

BTW, same for PostgreSQL ans SQLite. So all good.

will review this

Verified both the patches for drupal 9.5.x and drupal 10.
https://www.drupal.org/files/issues/2022-04-29/3215062-79--9.4.x.patch
https://www.drupal.org/files/issues/2022-04-29/3215062-79--10.0.x.patch

Testing Steps for Drupal 9:
* Install new Drupal Instance.
* Apply patch for Drupal 9.5
https://www.drupal.org/files/issues/2022-04-29/3215062-79--9.4.x.patch
* Run Update
drush updb

Testing Steps for Drupal 10:
* Install new Drupal Instance.
* Apply patch for drupal 10
https://www.drupal.org/files/issues/2022-04-29/3215062-79--10.0.x.patch
* Run Update
drush updb

Refer SS:
Drupal 9.5.x drush output:

Drupal 9.5.x flood table timestamp field declared as bigint:

Drupal 10 drush output:

Note: I have used lando setup and mysql database.

Test Result: Pass
Can be move to RTBC

+++ b/core/modules/comment/comment.install
@@ -116,3 +117,20 @@ function comment_schema() {
+
+/**
+ * Remove the year 2038 date limitation.
+ */
+function comment_update_9401(&$sandbox = NULL) {
+  $connection = \Drupal::database();
+  if ($connection->schema()->tableExists('comment_entity_statistics') && $connection->databaseType() != 'sqlite') {
+    $new = [

We're trying to avoid committing schema updates to 9.4.x or 9.5.x at this point, since it's an extra potential thing that can go wrong when updating from 9.4 to 10.0.

Patch therefore should be re-rolled against 10.1.x (i.e. comment_update_10100(), the first five-digit update number!).

A bit painful pushing this back a release but every time we have schema changes close to a major update something always goes wrong. And the patch can be committed to 10.1.x at any point from now onwards.

How's this? Interdiff relative to #67, even though that patch was numbered as "79".

Ugh, sorry. My local is busted right now for running 10.* tests. 😢 Let's hope the bot is happy with this one...

even though that patch was numbered as "79"

::facepalm::

I know I worked on the original patch, but my bit has been reviewed by @aarti zikre on #72 and @catch on #73. The rework on #76 is simply to address the rolling of the patch against 10.1.x.

Hopefully I won't get told off due RTBCing this :D

Committed 194b56c and pushed to 10.1.x. Thanks!

Getting this in nice and early will at least allow as much time as possible for any gremlins to be found.