MDEV-28782: modify mariadb-tzinfo-to-sql to set 'wsrep*' variables appropriately in cases where Galera is not compiled in

[dlenski]

Description

In 3b662c6, @dr-m appears to have discovered that the values of the wsrep_is_on and wsrep_cannot_replicate_tz variables need to be overridden for embedded builds.

However, there are other build configurations where these variables also
have NULL values.  The mariadb-tzinfo-to-sql script (implemented in
sql/tztime.cc) can be slightly modified to set its 'wsrep_is_on' and
'wsrep_cannot_replicate_tz' variables more predictably in all such cases,
thus allowing the mysql_tzinfo_to_sql_symlink.test test to pass without
any special-casing for particular build types.

See comments:

- https://github.com/MariaDB/server/commit/3b662c6ebd26b54ce534d9e7451cdc31e6c0046c#r78994411
- https://jira.mariadb.org/browse/MDEV-28782?focusedCommentId=230038&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-230038

How can this PR be tested?

All MTR tests, including mysql_tzinfo_to_sql_symlink.test, should still be passing in Buildbot as well as GitLab-CI, including in configurations where the wsrep_on and wsrep_mode variables do not exist.

Basing the PR against the correct MariaDB version

• This is a new feature and the PR is based against the latest MariaDB development branch
• This is a bug fix and the PR is based against the earliest branch in which the bug can be reproduced

10.6 is the earliest upstream branch containing 13e7793 (which led to this issue) and 3b662c6 (partial fix for the embedded-server case), so it appears to be the correct place to apply this change.

Backward compatibility

This is a fix for compatibility of a test, and should be fully backward-compatible.

---

All new code of the whole pull request, including one or several files that
are either new files or modified ones, are contributed under the BSD-new
license. I am contributing on behalf of my employer Amazon Web Services,
Inc.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[dlenski]

Ultimately, the fact that these variables can be null comes from the way that they are defined, as of 13e7793:

server/sql/tztime.cc

Lines 2738 to 2743 in 13e7793

	static const char *wsrep_is_on=
	"select sum(VARIABLE_NAME='wsrep_on' AND SESSION_VALUE='ON')"
	" from information_schema.SYSTEM_VARIABLES";
	static const char *wsrep_cannot_replicate_tz=
	"select sum(VARIABLE_NAME='wsrep_mode' and GLOBAL_VALUE NOT LIKE @replicate_opt)"
	" from information_schema.SYSTEM_VARIABLES";

server/sql/tztime.cc

Lines 2767 to 2781 in 13e7793

	printf("set @wsrep_is_on=(%s);\n", wsrep_is_on);
	printf("SELECT concat('%%', GROUP_CONCAT(OPTION), '%%') INTO @replicate_opt "
	" FROM"
	" (SELECT DISTINCT concat('REPLICATE_', UPPER(ENGINE)) AS OPTION"
	" FROM information_schema.TABLES"
	" WHERE TABLE_SCHEMA=DATABASE()"
	" AND TABLE_NAME IN ('time_zone',"
	" 'time_zone_name',"
	" 'time_zone_transition',"
	" 'time_zone_transition_type',"
	" 'time_zone_leap_second')"
	" AND ENGINE in ('MyISAM',"
	" 'Aria')) AS o"
	" ORDER BY OPTION DESC;\n");
	printf("set @wsrep_cannot_replicate_tz=@wsrep_is_on AND (%s);\n", wsrep_cannot_replicate_tz);

If Galera (aka "wsrep"?) is not compiled in, this variable doesn't exist at all, leading to @wsrep_is_on=NULL → @wsrep_cannot_replicate_tz=NULL.

So the cleanest and most robust fix would be to change those initial definitions to include coalesce(…, 0), and then get rid of the special cases in the test.

@grooverdan, do you agree with that? Is there any reason that any consumer of the tzinfo SQL would actually need to differentiate between "Galera is OFF" and "Galera is not compiled in"?

[grooverdan]

NULL is also a false behaviour in the sql standard, so I saw no need for coalesce, but you're right in that didn't consider this in the test flexibility.

Please add select version() like '%embedded%' into a or condition of $replace_wsrep_cols. I don't think you intentionally removed this?

[dlenski]

Please add select version() like '%embedded%' into a or condition of $replace_wsrep_cols. I don't think you intentionally removed this?

I did mean to remove that, actually. It seems to me to be error-prone and indirect to try to infer whether or not Galera is supported by looking at the version string, rather than simply looking at whether or not the Galera-related variables are populated (as I'm proposing here).

[dlenski]

@grooverdan, in 0e4cf49 you modified the SELECT query for wsrep_is_on to work around issues in bootstrap mode:

--- a/sql/tztime.cc
+++ b/sql/tztime.cc
@@ -2726,11 +2726,11 @@ static const char *trunc_tables_const=
   "TRUNCATE TABLE time_zone_transition;\n"
   "TRUNCATE TABLE time_zone_transition_type;\n";
 static const char *wsrep_is_on=
-  "select sum(VARIABLE_NAME='wsrep_on' AND SESSION_VALUE='ON')"
-  " from information_schema.SYSTEM_VARIABLES";
+  "select sum(SESSION_VALUE='ON')"
+  " from information_schema.SYSTEM_VARIABLES WHERE VARIABLE_NAME='wsrep_on'";
 static const char *wsrep_cannot_replicate_tz=
-  "select sum(VARIABLE_NAME='wsrep_mode' and GLOBAL_VALUE NOT LIKE @replicate_opt)"
-  " from information_schema.SYSTEM_VARIABLES";
+  "select sum(GLOBAL_VALUE NOT LIKE @replicate_opt)"
+  " from information_schema.SYSTEM_VARIABLES WHERE VARIABLE_NAME='wsrep_mode'";
 
 int
 main(int argc, char **argv)

… however this same change is also what caused wsrep_is_on to stop getting properly populated when Galera/wsrep is entirely removed from the build.

[dlenski]

@grooverdan
In 6d48c6a, I've updated the PR to use the simpler approach described in #2196 (comment), which removes the need for any special-casing based on version-string or build type.

[grooverdan]

please record test case results.

[dlenski]

@grooverdan wrote:

please record test case results.

Derp, thank you 🤦‍♂️. Fixed and repushed…