Bar, please review bb-11.3-monty

monty, there is a summary of my review comments (previously discussed on slack):

• This code is going to change under terms of MDEV-32496 and MDEV-10018. Therefore, it's better to take into account planned changes, to stabilize the underlying code one time instead of two times.

• I don't like that you use a differently defined my_time_t on different platforms. If you want to preserve my_time_t for plugin compatibility, then the server code should use a different data type. Let's call it for example my_seconds_t, and it should be defines as longlong (signed).

• This new data type for seconds should be a part of ABI.

• The type my_time_t should not be touched, and it should stay in ABI. I don't like that you removed it. I think this is meaningless to remove my_time_t from ABI but insist that we need it for backward compatibility.

• The data type in most server files should be changed from my_time_t to my_seconds_t. Functions/methods which depend on my_time_t that you want still to be available for plugings should be duplicated: for my_time_t and for my_seconds_t.

• my_timeval should be changed as follows:

  struct my_timeval

  {

  longlong tv_sec; -- change to my_seconds_t

  long tv_usec; -- change to uint32

  };

• To convert between my_seconds_t and MYSQL_TIME on more 32bit systems, we should add a CMake check for functions localtime64_r (Unix) and locatime64_s (Windows) and use these functions when available.

The patches in bb-11.3-monty are Ok to push.
I'd still prefer though if we used a system independent replacement for my_time_t.

The branch is now bb-11.4-timestamp.

I encountered the changeset Updated ALTER TABLE to convert old row_end timestamps to new timestamp range that is related to this task. I think that a change of a data type (from unsigned 31-bit timestamps to unsigned 32-bit) needs to be signaled to the storage engines by setting the ALTER_STORED_COLUMN_TYPE flag. In that way, InnoDB will refuse to perform any native ALTER TABLE operation just fine, and any fixup attempts like this one should be unnecessary. The idea of handler::check_if_supported_inplace_alter() is to let the storage engine choose the most efficient algorithm, unless the user has instructed otherwise, by specifying ALGORITHM=COPY or setting the parameters old_alter_table=1 or (ever since MDEV-13134) alter_algorithm=copy.

The ALTER TABLE attribute FORCE was originally introduced in MySQL 5.6.17 by the following bug fix, to allow the MySQL 5.6.8 WL#6255 online table rebuild to work on OPTIMIZE TABLE:
BUG# 13975227: ONLINE OPTIMIZE TABLE FOR INNODB TABLES
The idea is that OPTIMIZE TABLE will map to ALTER TABLE…FORCE in the case that handler::optimize() is not implemented or supported in the storage engine.

I do not know which was the first GA release of MariaDB Server 10.0, but I think that this change was part of that release series from rather early on.

I reviewed the InnoDB changes, and they look OK to me.

The changes in REPAIR and ALTER FORCE should have undergone the normal testing cycle as they are generally unrelated to this task and in themselves form a feature, now filed as MDEV-33449. However, this feature is required for the current implementation of this task.

Several last pushes into the branch weren't building on some platforms in buildbot and the failed compilation was partially masking test results.
Now we can see failures in buildbot apparently related to the REPAIR changes, e.g.
https://buildbot.mariadb.net/buildbot/builders/winx64-packages/builds/42997/steps/test/logs/stdio

CURRENT_TEST: main.ctype_utf8_def_upgrade

--- D:/winx64-packages/build/src/mysql-test/main/ctype_utf8_def_upgrade.result	2024-02-12 22:41:54.000000000 +0200

+++ D:\winx64-packages\build\src\mysql-test\main\ctype_utf8_def_upgrade.reject	2024-02-12 22:52:32.251677600 +0200

@@ -28,7 +28,7 @@

 REPAIR TABLE t1 USE_FRM;

 Table	Op	Msg_type	Msg_text

 test.t1	repair	note	Table data recovered

-t1	repair	error	Table rebuild required. Please do "ALTER TABLE `test.t1` FORCE" or dump/reload to fix it!

+t1	repair	error	Table rebuild required. Please do "ALTER TABLE `???/` FORCE" or dump/reload to fix it!

 test.t1	repair	status	Operation failed

 SELECT COUNT(*) FROM t1;

 COUNT(*)

@@ -83,7 +83,7 @@

 REPAIR TABLE t1 USE_FRM;

 Table	Op	Msg_type	Msg_text

 db1.t1	repair	note	Table data recovered

-t1	repair	error	Table rebuild required. Please do "ALTER TABLE `db1.t1` FORCE" or dump/reload to fix it!

+t1	repair	error	Table rebuild required. Please do "ALTER TABLE `` FORCE" or dump/reload to fix it!

 db1.t1	repair	status	Operation failed

 SELECT COUNT(*) FROM t1;

 COUNT(*)

Maybe the test failures would be easy to fix, but they look much like a corruption and demonstrate that there is room for concern.

Between this evidence and very late changes which may affect much more than just timestamp conversion, my suggestion is to hold both features (MDEV-32188 and MDEV-33449) till the next release and meanwhile let it be tested more.

This unfortunately prevents us from getting feedback from the community, which would be useful as various 3rd-party tools could have revealed unpredicted effects of the timestamp-related changes. We should probably put it into 11.5 preview release again, even though we don't usually do that (e.g. we could try to get it pushed into 11.5 main branch before the preview release, in this case it will get to the preview release automatically).

to test this feature separately please use bb-11.5-MDEV-32188-timestamps

As of bb-11.5-MDEV-32188-timestamps ee224ef3a, I have no objections against pushing the feature into 11.5 main and releasing with 11.5.1 RC. The branch contains two other features which are pre-requisites for this one, MDEV-33655 (Remove alter_algorithm) and MDEV-33449 (improving repair of tables), they were commented upon in the corresponding JIRA tasks; notes here are limited to this task only.

I think it is beneficial to release the feature to the community well in advance before the next LTS, as the real-life usage can reveal some issues (for example, in relation to third-party products) which we haven't foreseen.
For now, we know of at least the following points which users should be aware of:

1) If a logical dump was taken from an old server by an old version of mariadb-dump client, restoring such a dump on a new server may cause apparent loss of current data in system-versioned tables. In reality the data will still be there, but it will be treated as historical instead of current. In particular it can happen if

• the dump was taken with --dump-history, or
• the system-versioned table has explicitly defined row start / row end columns.

One of possible workarounds is to use the new mariadb-dump client with --update-history option.
The issue was originally described as a part of MDEV-33032 which is now closed, but there is a comment that --update-history option may need to be backported to earlier versions, it hasn't been yet.

2) There can be a variety of scenarios when replication from an old primary to a new replica involving timestamps from the extended range would cause data divergence, and eventually replication abort. Normally an old server shouldn't be trying to insert out-of-range timestamps into the tables, but it can happen inadvertently, for example when both datetime and timestamp are involved, etc. A few of such cases are described in MDEV-34121.

3) mariadb-binlog cannot parse events written with system timestamp 4294967295 (MDEV-33239). This issue is very unlikely to affect real-life usage, unless the timestamp was set to a huge value by operational mistake or by malice (the latter can be prevented by using secure_timestamp option). However, should it happen, the result is very confusing, because while the data is still there in the binary log, mariadb-binlog will truncate it without a warning; and if its output is further fed to a server, the data can be lost. For testing, it can be quite a nuisance.

4) Columnstore does not support the extended timestamps (MDEV-33302, MCOL-5644).