Hi, Rucha, Few comments, after I saw the commit in the branch: On Sep 04, Rucha Deodhar wrote:

revision-id: 8114fb63bbb (mariadb-11.0.1-229-g8114fb63bbb) parent(s): 7ba9c7fb84b author: Rucha Deodhar committer: Rucha Deodhar timestamp: 2023-09-01 11:01:09 +0530 message:

MDEV-31684: Add timezone information to DATE_FORMAT

Before starting to go over the format string, prepare the current time zone information incase '%z' or '%Z' is encountered. This information can be obtained as given below:

A) If timezone is not set ( meaning we are working with system timezone): Get the MYSQL_TIME representation for current time and GMT time using current thread variable for timezone and timezone variable for UTC respectively. This MYSQL_TIME variable will be used to calculate time difference. Also convert current time in second to tm structure to get system timezone information.

B) If timezone is set as offset: Get timezone information using current timezone information and store in appropriate variable.

C) If timezone is set as some place (example: Europe/Berlin) Get timezone information by searching the timezone. During internal timezone search, information like timeoffset from UTC and abbrevation is stored in another relevant structure. Hence use the same information.

diff --git a/mysql-test/main/date_formats.result b/mysql-test/main/date_formats.result index 463cce39520..23aeffedef7 100644 --- a/mysql-test/main/date_formats.result +++ b/mysql-test/main/date_formats.result @@ -573,3 +573,67 @@ time_format('01 02:02:02', '%T') select time_format('2001-01-01 02:02:02', '%T'); time_format('2001-01-01 02:02:02', '%T') 02:02:02 +# +# Beginning of 11.3 test +# +# MDEV-31684: Add timezone information to DATE_FORMAT +# +SET @old_timezone= @@time_zone; +SET TIME_ZONE='Japan'; +SELECT DATE_FORMAT('2009-10-04 22:23:00', '%z %Z') AS current_timezone; +current_timezone ++0900 CJT +SELECT DATE_FORMAT(CURDATE(), '%z %Z') AS current_tz;

1) please, don't use CURDATE/CURRENT_TIMESTAMP/etc. The time zone abbreviation depends on the date: $ TZ=Europe/Berlin date -d '2020-01-01 1:2:3' Wed Jan 1 01:02:03 CET 2020 $ TZ=Europe/Berlin date -d '2020-06-01 1:2:3' Mon Jun 1 01:02:03 CEST 2020 See? Could be CET or CEST. Japan doesn't seem to have daylight saving, so it'll be the same abbreviation throughout the year, but it can still change at some point. So always use a fixed date, like in your first test. 2) why is it CJT at all? $ TZ=Japan date -d '2009-10-04 22:23:00' +'%z %Z' +0900 JST it looks like it should be JST? ... 3) also, please test that CET/CEST example. Or, rather, MET/MEST/MST/MMT example - the data for these abbreviations are already present in mariadb_test_data_timezone.sql That is, add lines like SET @@time_zone='Europe/Moscow'; SELECT DATE_FORMAT('....', '%z %Z'); SELECT DATE_FORMAT('....', '%z %Z'); SELECT DATE_FORMAT('....', '%z %Z'); and with different dates it should return different abbreviations for the same time zone.

+# +# output depends on system time information. Hence replacing +# to avoid result diff test failures. +# +SET @@time_zone= default; +SELECT DATE_FORMAT('2009-10+|-HH:MM ABC22:23:00', '%z %Z') AS current_timezone;

1) I don't understand what you're testing here :( 2) Anyway, note that you can put, say, --timezone=Europe/Berlin into the date_formats.opt file and the server will be restarted with this value in TZ. That is, you can force the system time zone to be a specific fixed value.

diff --git a/scripts/mariadb_test_data_timezone.sql b/scripts/mariadb_test_data_timezone.sql index 8d07d413cef..83bb548478d 100644 --- a/scripts/mariadb_test_data_timezone.sql +++ b/scripts/mariadb_test_data_timezone.sql @@ -14,8 +14,8 @@ -- along with this program; if not, write to the Free Software -- Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA

-INSERT INTO time_zone_name (Name, Time_Zone_id) ... -INSERT INTO time_zone (Time_zone_id, Use_leap_seconds) ... -INSERT INTO time_zone_transition (Time_zone_id, Transition_time, Transition_type_id) ... -INSERT INTO time_zone_transition_type (Time_zone_id, Transition_type_id, `Offset`, Is_DST, Abbreviation) ... -INSERT INTO time_zone_leap_second (Transition_time, Correction) ... +INSERT INTO time_zone_name (Name, Time_Zone_id) VALUES ... ('India/Kolkata', 6); +INSERT INTO time_zone (Time_zone_id, Use_leap_seconds) ... +INSERT INTO time_zone_transition (Time_zone_id, Transition_time, Transition_type_id) ... +INSERT INTO time_zone_transition_type (Time_zone_id, Transition_type_id, `Offset`, Is_DST, Abbreviation) ... +INSERT INTO time_zone_leap_second (Transition_time, Correction) ...

1. Why? I didn't see where you use this new value in tests. 2. Is it really India/Kolkata? The official https://www.iana.org/time-zones seems to have Asia/Kolkata.

diff --git a/sql/item_timefunc.cc b/sql/item_timefunc.cc index 26adc4eddaa..f8d3fe7dfe8 100644 --- a/sql/item_timefunc.cc +++ b/sql/item_timefunc.cc @@ -468,20 +468,111 @@ static bool extract_date_time(THD *thd, DATE_TIME_FORMAT *format, DBUG_RETURN(1); }

+/* + + Gets local time in MYSQL_TIME structure and tm struct. + +*/ +void get_timezone_offset_information(THD *thd, Time_zone *tz, int &hr, + int &min, char* abbrevation, + bool &is_neg) +{ + if (!tz) + return; + + if (tz->get_name()->length() == tz_SYSTEM_name.length() && + !strncmp((const char*)(tz->get_name()->ptr()), + (const char*)(tz_SYSTEM_name.ptr()), 6)) + { + struct tm tm_local_time; + time_t time_sec= my_time(0); + MYSQL_TIME local_TIME, gmt_TIME; + ulonglong seconds_diff; + ulong microsec_diff; + + /* Get local time in MYSQL_TIME struct. */ + thd->variables.time_zone->gmt_sec_to_TIME(&local_TIME, + thd->query_start()); + /* Get GMT time in MYSQL_TIME struct. */ + my_tz_UTC->gmt_sec_to_TIME(&gmt_TIME, thd->query_start()); + + /* We need above two MYSQL_TIME to get time difference. */ + is_neg= calc_time_diff(&local_TIME, &gmt_TIME, 1, + &seconds_diff, µsec_diff); + + /* + once we have time difference in seconds_diff, + convert it into hr and min. + */ + hr= (int)(seconds_diff/3600L); + int temp= (int)(seconds_diff%3600L); + min= temp/60L; + + /* + Get timezone abbrevation. It is stored in tm structure, + so convert time to tm struct and copy it. + */

1) tm structure also has the offset, you don't need to calculate it manually. 2) you should use not my_time(0) but the value of the first DATE_FORMAT() argument

+ localtime_r(&time_sec, &tm_local_time); +# ifdef __USE_MISC + int len= strlen((tm_local_time.tm_zone)); + strncpy(abbrevation, (tm_local_time.tm_zone), len); + abbrevation[len]= '\0'; +# endif + } + else + { + /* + Information for "named" timezone and offset can be obtained from their + relevant class. So get the timezone information first. + */ + struct tz tmp; + tz->get_timezone_information(&tmp); + is_neg= tmp.is_behind; + + /* Get time in hour and min. */ + time_t time_in_sec= abs(tmp.seconds_offset); + hr= (int)(time_in_sec/3600L); + int temp= (int)(time_in_sec%3600L); + min= temp/60L; + + /* Copy the abbrevation. */ + strncpy(abbrevation, (const char*)(tmp.abbrevation), + strlen((const char*)(tmp.abbrevation))); + abbrevation[strlen((const char*)(tmp.abbrevation))+1]='\0'; + } + + return; +} +

/** Create a formatted date/time value in a string. */

-static bool make_date_time(const String *format, const MYSQL_TIME *l_time, - timestamp_type type, const MY_LOCALE *locale, - String *str) +static bool make_date_time(THD *thd, const String *format, + const MYSQL_TIME *l_time, timestamp_type type, + const MY_LOCALE *locale, String *str) { char intbuff[15]; uint hours_i; uint weekday; ulong length; const char *ptr, *end; + int diff_hr=0, diff_min=0; + bool curr_time_behind_utc= false; + char abbrevation[8]; + + Time_zone* curr_timezone= my_tz_find(thd, + thd->variables.time_zone->get_name()); + + /* + precalculate local time and timezone information because + %z or %Z maybe present in the format. + */ + memset(abbrevation, 0, sizeof(abbrevation)); + get_timezone_offset_information(thd, curr_timezone, diff_hr, + diff_min, abbrevation, + curr_time_behind_utc);

Just a thought. get_timezone_offset_information() doesn't seem to be very cheap. May be do it only when %z or %Z is requested? Like case 'z': if (!have_tz_info) { memset(abbrevation, 0, sizeof(abbrevation)); get_timezone_offset_information(thd, curr_timezone, diff_hr, diff_min, abbrevation, curr_time_behind_utc); have_tz_info= true; } Or better something with less code duplication :)

str->length(0);

@@ -699,6 +790,31 @@ static bool make_date_time(const String *format, const MYSQL_TIME *l_time, str->append_with_prefill(intbuff, length, 1, '0'); break;

+ case 'z': + { + if (curr_time_behind_utc) + str->append("-", 1, system_charset_info); + else + str->append("+", 1, system_charset_info); + + if (diff_hr/10 == 0) + str->append("0", 1, system_charset_info); + length= (uint) (int10_to_str(diff_hr, intbuff, 10) - intbuff); + str->append_with_prefill(intbuff, length, 1, '0'); + if (diff_min/10 == 0) + str->append("0", 1, system_charset_info); + length= (uint) (int10_to_str(diff_min, intbuff, 10) - intbuff); + str->append_with_prefill(intbuff, length, 1, '0');

why do you append "0" manually, if you use append_with_prefill(), which, I suppose, does it automatically? or, if you add "0" manually, you should use simple append().

+ } + break; + + case 'Z': + { + str->append(abbrevation, + (uint) strlen(abbrevation), + system_charset_info); + } + break; default: str->append(*ptr); break; @@ -1912,7 +2028,7 @@ String *Item_func_date_format::val_str(String *str)

/* Create the result string */ str->set_charset(collation.collation); - if (!make_date_time(format, &l_time, + if (!make_date_time(thd, format, &l_time, is_time_format ? MYSQL_TIMESTAMP_TIME : MYSQL_TIMESTAMP_DATE, lc, str)) diff --git a/sql/tztime.cc b/sql/tztime.cc index e77a7529332..4662fa082e0 100644 --- a/sql/tztime.cc +++ b/sql/tztime.cc @@ -55,15 +55,10 @@

/* Now we don't use abbreviations in server but we will do this in future. + Edit: Startd needing abbrevation in server as part of task MDEV-31684.

typo

*/ -#if defined(TZINFO2SQL) || defined(TESTTIME) -#define ABBR_ARE_USED -#else -#if !defined(DBUG_OFF) -/* Let use abbreviations for debug purposes */ -#undef ABBR_ARE_USED -#define ABBR_ARE_USED -#endif /* !defined(DBUG_OFF) */ +#ifndef ABBR_ARE_USED +#define ABBR_ARE_USED #endif /* defined(TZINFO2SQL) || defined(TESTTIME) */

/* Structure describing local time type (e.g. Moscow summer time (MSD)) */ @@ -1008,11 +1003,6 @@ TIME_to_gmt_sec(const MYSQL_TIME *t, const TIME_ZONE_INFO *sp, uint *error_code)

#if !defined(TESTTIME) && !defined(TZINFO2SQL)

-/* - String with names of SYSTEM time zone. -*/ -static const String tz_SYSTEM_name("SYSTEM", 6, &my_charset_latin1); -

/* Instance of this class represents local time zone used on this system @@ -1031,6 +1021,7 @@ class Time_zone_system : public Time_zone virtual my_time_t TIME_to_gmt_sec(const MYSQL_TIME *t, uint *error_code) const; virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; virtual const String * get_name() const; + virtual void get_timezone_information(struct tz* curr_tz) const; };

@@ -1097,6 +1088,12 @@ Time_zone_system::gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const adjust_leap_second(tmp); }

+void +Time_zone_system::get_timezone_information(struct tz* curr_tz) const +{

this is weird. You have a special if (tz->get_name()->length() == tz_SYSTEM_name.length() && !strncmp((const char*)(tz->get_name()->ptr()), (const char*)(tz_SYSTEM_name.ptr()), 6)) to detect SYSTEM time zone by name in get_timezone_offset_information(). why not to put that logic here, in the dedicated method, like you did for other time zones?

+ return; +} +

/* Get name of time zone @@ -1128,6 +1125,7 @@ class Time_zone_utc : public Time_zone uint *error_code) const; virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; virtual const String * get_name() const; + virtual void get_timezone_information(struct tz* curr_tz) const; };

@@ -1175,6 +1173,12 @@ Time_zone_utc::gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const adjust_leap_second(tmp); }

+void +Time_zone_utc::get_timezone_information(struct tz* curr_tz) const +{

what is printed for UTC? Please, add a test for that.

+ return; +} +

/* Get name of time zone @@ -1210,6 +1214,7 @@ class Time_zone_db : public Time_zone virtual my_time_t TIME_to_gmt_sec(const MYSQL_TIME *t, uint *error_code) const; virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; virtual const String * get_name() const; + virtual void get_timezone_information(struct tz* curr_tz) const; private: TIME_ZONE_INFO *tz_info; const String *tz_name; @@ -1280,6 +1285,16 @@ Time_zone_db::gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const adjust_leap_second(tmp); }

+void +Time_zone_db::get_timezone_information(struct tz* curr_tz) const +{ + curr_tz->seconds_offset= tz_info->revtis->rt_offset; + curr_tz->is_behind= tz_info->revtis->rt_offset < 0 ? true : false; + strncpy((char*)(curr_tz->abbrevation), (const char*)(tz_info->chars), + strlen((const char*)(tz_info->chars))+1); + curr_tz->abbrevation[strlen((const char*)(tz_info->chars))+1]='\0';

here and below (in Time_zone_offset) 1. why casts? 2. you use strncpy incorrectly, the third argument specifies the size of the buffer, like strncpy(curr_tz->abbrevation, tz_info->chars, sizeof(curr_tz->abbrevation)); but better use strmake, it'll do the zero-termination for you strmake(curr_tz->abbrevation, tz_info->chars, sizeof(curr_tz->abbrevation)-1);

+} +

/* Get name of time zone @@ -1309,6 +1324,7 @@ class Time_zone_offset : public Time_zone uint *error_code) const; virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; virtual const String * get_name() const; + virtual void get_timezone_information(struct tz* curr_tz) const; /* This have to be public because we want to be able to access it from my_offset_tzs_get_key() function @@ -1434,6 +1450,16 @@ Time_zone_offset::get_name() const return &name; }

+void +Time_zone_offset::get_timezone_information(struct tz* curr_tz) const +{ + curr_tz->seconds_offset= offset; + const char *name= get_name()->ptr(); + curr_tz->is_behind= name[0] == '+' ? false : true; + strncpy((char*)(curr_tz->abbrevation), name, this->get_name()->length()); + curr_tz->abbrevation[this->get_name()->length()]= '\0'; +} +

static Time_zone_utc tz_UTC; static Time_zone_system tz_SYSTEM; diff --git a/sql/tztime.h b/sql/tztime.h index 6d8af62ecd4..57e4aa2021b 100644 --- a/sql/tztime.h +++ b/sql/tztime.h @@ -26,6 +26,11 @@ #include "sql_list.h" /* Sql_alloc */ #include "sql_string.h" /* String */

+/* + String with names of SYSTEM time zone. +*/ +static const String tz_SYSTEM_name("SYSTEM", 6, &my_charset_latin1);

No, sorry. It's a .h file included in many .cc files. You'll have a separate `static const String` in every single source file that includes this header.

+ class THD;

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org