Re: fec3696fef6: MDEV-15751 CURRENT_TIMESTAMP should return a TIMESTAMP [WITH TIME ZONE?]
Hi, Alexander, This has waited far too long :(
commit fec3696fef6 Author: Alexander Barkov <bar@mariadb.com> Date: Tue Feb 14 13:27:46 2023 +0400
diff --git a/sql/sql_class.h b/sql/sql_class.h index f0bf695a1c1..7378225eaa2 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3900,7 +3900,8 @@ class THD: public THD_count, /* this must be first */ } const Type_handler *type_handler_for_datetime() const; bool timestamp_to_TIME(MYSQL_TIME *ltime, my_time_t ts, - ulong sec_part, date_mode_t fuzzydate); + ulong sec_part, date_mode_t fuzzydate, + bool zero_timesteamp_means_zero_datetime);
I don't understand it. NOW() cannot return a zero timestamp, how this feature can possibly need changes to zero timestamp treatment?
inline my_time_t query_start() { return start_time; } inline ulong query_start_sec_part() { used|= QUERY_START_SEC_PART_USED; return start_time_sec_part; } diff --git a/sql/item_timefunc.h b/sql/item_timefunc.h index a5f6d9307c6..1c463c8a332 100644 --- a/sql/item_timefunc.h +++ b/sql/item_timefunc.h @@ -707,6 +707,49 @@ class Item_datetimefunc :public Item_func };
+class Item_timestampfunc: public Item_func +{ +protected: + Datetime to_datetime(THD *thd) + { + return Timestamp_or_zero_datetime_native_null(thd, this).to_datetime(thd);
how does it differ from Datetime(this) ?
+ } +public: + Item_timestampfunc(THD *thd): Item_func(thd) {} + Item_timestampfunc(THD *thd, Item *a): Item_func(thd, a) {} + const Type_handler *type_handler() const override + { return &type_handler_timestamp2; } + bool get_date(THD *thd, MYSQL_TIME *ltime, date_mode_t fuzzydate) override + { + return null_value= to_datetime(thd).copy_to_mysql_time(ltime); + } + double val_real() override + { + Datetime dt= to_datetime(current_thd); + null_value= !dt.is_valid_datetime(); + return dt.to_double();
may be `return null_value ? 0 : dt.to_double()` ? why to call to_double if it isn't a valid datetime? (same everywhere below)
+ } + longlong val_int() override + { + Datetime dt= to_datetime(current_thd); + null_value= !dt.is_valid_datetime(); + return dt.to_longlong(); + } + my_decimal *val_decimal(my_decimal *to) override + { + Datetime dt= to_datetime(current_thd); + null_value= !dt.is_valid_datetime(); + return dt.to_decimal(to); + } + String *val_str(String *to) override + { + Datetime dt= to_datetime(current_thd); + null_value= !dt.is_valid_datetime(); + return dt.to_string(to, decimals); + } +}; + + /* Abstract CURTIME function. Children should define what time zone is used */
class Item_func_curtime :public Item_timefunc @@ -837,20 +880,48 @@ class Item_func_now :public Item_datetimefunc };
-class Item_func_now_local :public Item_func_now +/* + This is a replacement for Item_func_now_local, + returning TIMESTAMP instead of DATETIME.
why did you rename it? why didn't you change Item_func_now_utc?
+*/ +class Item_func_current_timestamp: public Item_timestampfunc { public: - Item_func_now_local(THD *thd, uint dec): Item_func_now(thd, dec) {} + Item_func_current_timestamp(THD *thd, uint dec) + :Item_timestampfunc(thd) + { decimals= dec; }
why not Item_timestampfunc(thd, dec) ?
LEX_CSTRING func_name_cstring() const override { static LEX_CSTRING name= {STRING_WITH_LEN("current_timestamp") }; return name; } - int save_in_field(Field *field, bool no_conversions) override; - void store_now_in_TIME(THD *thd, MYSQL_TIME *now_time) override; + void print(String *str, enum_query_type query_type) override + { + str->append(func_name_cstring()); + str->append('('); + if (decimals) + str->append_ulonglong(decimals); + str->append(')'); + } + bool fix_length_and_dec(THD *thd) override + { + if (check_decimal_scale_or_error(TIME_SECOND_PART_DIGITS)) + return true; + fix_attributes_datetime(decimals); + return false; + } + bool val_native(THD *thd, Native *to) override; + bool check_vcol_func_processor(void *arg) override + { + /* + NOW is safe for replication as slaves will run with same time as + master + */
what does this replication comment do in check_vcol_func_processor? (I know it was in the old code too, but "old code did it" never stopped your cleanups before :)
+ return mark_unsupported_function(func_name(), "()", arg, VCOL_TIME_FUNC); + } enum Functype functype() const override { return NOW_FUNC; } Item *get_copy(THD *thd) override - { return get_item_copy<Item_func_now_local>(thd, this); } + { return get_item_copy<Item_func_current_timestamp>(thd, this); } };
diff --git a/sql/item_timefunc.cc b/sql/item_timefunc.cc index 52b9ae7a682..3e224e3e6f7 100644 --- a/sql/item_timefunc.cc +++ b/sql/item_timefunc.cc @@ -1642,32 +1633,10 @@ void Item_func_now::print(String *str, enum_query_type query_type) }
-int Item_func_now_local::save_in_field(Field *field, bool no_conversions) +bool Item_func_current_timestamp::val_native(THD *thd, Native *to) { - if (field->type() == MYSQL_TYPE_TIMESTAMP) - { - THD *thd= field->get_thd(); - my_time_t ts= thd->query_start(); - ulong sec_part= decimals ? thd->query_start_sec_part() : 0; - sec_part-= my_time_fraction_remainder(sec_part, decimals); - field->set_notnull(); - field->store_timestamp(ts, sec_part); - return 0; - } - else - return Item_datetimefunc::save_in_field(field, no_conversions); -} - - -/** - Converts current time in my_time_t to MYSQL_TIME representation for local - time zone. Defines time zone (local) used for whole NOW function. -*/ -void Item_func_now_local::store_now_in_TIME(THD *thd, MYSQL_TIME *now_time) -{ - thd->variables.time_zone->gmt_sec_to_TIME(now_time, thd->query_start()); - set_sec_part(thd->query_start_sec_part(), now_time, this); - thd->used|= THD::TIME_ZONE_USED; + Timestamp ts(Timeval(thd->query_start(), thd->query_start_sec_part())); + return null_value= ts.trunc(decimals).to_native(to, decimals);
can this possibly fail?
}
@@ -2732,7 +2693,6 @@ String *Item_func_tochar::val_str(String* str)
bool Item_func_from_unixtime::fix_length_and_dec(THD *thd) { - thd->used|= THD::TIME_ZONE_USED;
would be nice to have a test case for this. Like direct insert into a timestamp column no longer uses timezone - e.g. timestamp within dst switch time is inserted correctly. and vice versa, when from_unixtime is converted to a string, like `insert ... values(concat(from_unixtime(...)))` then TIME_ZONE_USED is still set and show binlog events (or mysqlbinlog) should show that the timezone was correctly binlogged, as before.
tz= thd->variables.time_zone; Type_std_attributes::set( Type_temporal_attributes_not_fixed_dec(MAX_DATETIME_WIDTH,
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hello Sergei, Thank you for the review! A new patch is here: https://github.com/MariaDB/server/commit/b626f5398dccccab4a01e20eb59778c5163... As agreed, this patch version preserves the old implementation of Item_func_now_local under SQL function name LOCALTIMESTAMP. Please see comments below: On 8/22/24 19:33, Sergei Golubchik wrote:
Hi, Alexander,
This has waited far too long :(
commit fec3696fef6 Author: Alexander Barkov <bar@mariadb.com> Date: Tue Feb 14 13:27:46 2023 +0400
diff --git a/sql/sql_class.h b/sql/sql_class.h index f0bf695a1c1..7378225eaa2 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3900,7 +3900,8 @@ class THD: public THD_count, /* this must be first */ } const Type_handler *type_handler_for_datetime() const; bool timestamp_to_TIME(MYSQL_TIME *ltime, my_time_t ts, - ulong sec_part, date_mode_t fuzzydate); + ulong sec_part, date_mode_t fuzzydate, + bool zero_timesteamp_means_zero_datetime);
I don't understand it. NOW() cannot return a zero timestamp, how this feature can possibly need changes to zero timestamp treatment?
In the new patch reduction this change is not needed any more. I removed it. It was needed in the old reduction, because FROM_UNIXTIME(0) returned '1970-01-01 00:00:00'. I did not check for sure though - it's not important any more.
inline my_time_t query_start() { return start_time; } inline ulong query_start_sec_part() { used|= QUERY_START_SEC_PART_USED; return start_time_sec_part; } diff --git a/sql/item_timefunc.h b/sql/item_timefunc.h index a5f6d9307c6..1c463c8a332 100644 --- a/sql/item_timefunc.h +++ b/sql/item_timefunc.h @@ -707,6 +707,49 @@ class Item_datetimefunc :public Item_func };
+class Item_timestampfunc: public Item_func +{ +protected: + Datetime to_datetime(THD *thd) + { + return Timestamp_or_zero_datetime_native_null(thd,
this).to_datetime(thd);
how does it differ from Datetime(this) ?
Using Datetime(this) would cause an infinite recursion here. Datetime(item) always uses item->get_date() to construct the value, not matter what the Item data type is. Datetime does not know about the TIMESTAMP data type. Fixing Datetime to catch a TIMESTAMP input and to use val_native() followed by a constructing Datetime from a timeval value would fix the recursion problem. But I think it's not needed. This sequence of converting the data type is just more clear than anything else: 1. Construct the instance of the native data type representation, Timestamp_or_zero_datetime_native_null in this case. 2. Convert this instance to another type using its conversion method. to_datetime() in this case.
+ } +public: + Item_timestampfunc(THD *thd): Item_func(thd) {} + Item_timestampfunc(THD *thd, Item *a): Item_func(thd, a) {} + const Type_handler *type_handler() const override + { return &type_handler_timestamp2; } + bool get_date(THD *thd, MYSQL_TIME *ltime, date_mode_t fuzzydate) override + { + return null_value= to_datetime(thd).copy_to_mysql_time(ltime); + } + double val_real() override + { + Datetime dt= to_datetime(current_thd); + null_value= !dt.is_valid_datetime(); + return dt.to_double();
may be `return null_value ? 0 : dt.to_double()` ? why to call to_double if it isn't a valid datetime? (same everywhere below)
class Datetime already implements methods like to_double() as follows: double to_double() const { return !is_valid_datetime() ? 0 : Temporal::to_double(neg, TIME_to_ulonglong_datetime(this), second_part); }
+ } + longlong val_int() override + { + Datetime dt= to_datetime(current_thd); + null_value= !dt.is_valid_datetime(); + return dt.to_longlong(); + } + my_decimal *val_decimal(my_decimal *to) override + { + Datetime dt= to_datetime(current_thd); + null_value= !dt.is_valid_datetime(); + return dt.to_decimal(to); + } + String *val_str(String *to) override + { + Datetime dt= to_datetime(current_thd); + null_value= !dt.is_valid_datetime(); + return dt.to_string(to, decimals); + } +}; + + /* Abstract CURTIME function. Children should define what time
zone is used */
class Item_func_curtime :public Item_timefunc @@ -837,20 +880,48 @@ class Item_func_now :public Item_datetimefunc };
-class Item_func_now_local :public Item_func_now +/* + This is a replacement for Item_func_now_local, + returning TIMESTAMP instead of DATETIME.
why did you rename it? why didn't you change Item_func_now_utc?
The new patch reduction: - Implements Item_func_current_timestamp under SQL name CURRENT_TIMESTAMP. - Keeps the old class Item_func_now_local under a new SQL name LOCALTIMESTAMP Not sure if we should rename classes Item_func_now_local and Item_func_now_utc to match their SQL names. For easier merge purposes perhaps we should not. Also some storage engines can use these class names.
+*/ +class Item_func_current_timestamp: public Item_timestampfunc { public: - Item_func_now_local(THD *thd, uint dec): Item_func_now(thd, dec) {} + Item_func_current_timestamp(THD *thd, uint dec) + :Item_timestampfunc(thd) + { decimals= dec; }
why not Item_timestampfunc(thd, dec) ?
I have added a new version of the Item_timestampfun constructor and fixed this code as you suggest.
LEX_CSTRING func_name_cstring() const override { static LEX_CSTRING name= {STRING_WITH_LEN("current_timestamp") }; return name; } - int save_in_field(Field *field, bool no_conversions) override; - void store_now_in_TIME(THD *thd, MYSQL_TIME *now_time) override; + void print(String *str, enum_query_type query_type) override + { + str->append(func_name_cstring()); + str->append('('); + if (decimals) + str->append_ulonglong(decimals); + str->append(')'); + } + bool fix_length_and_dec(THD *thd) override + { + if (check_decimal_scale_or_error(TIME_SECOND_PART_DIGITS)) + return true; + fix_attributes_datetime(decimals); + return false; + } + bool val_native(THD *thd, Native *to) override; + bool check_vcol_func_processor(void *arg) override + { + /* + NOW is safe for replication as slaves will run with same time as + master + */
what does this replication comment do in check_vcol_func_processor? (I know it was in the old code too, but "old code did it" never stopped your cleanups before :)
Removed.
+ return mark_unsupported_function(func_name(), "()", arg,
+ } enum Functype functype() const override { return NOW_FUNC; } Item *get_copy(THD *thd) override - { return get_item_copy<Item_func_now_local>(thd, this); } + { return get_item_copy<Item_func_current_timestamp>(thd, this); } };
diff --git a/sql/item_timefunc.cc b/sql/item_timefunc.cc index 52b9ae7a682..3e224e3e6f7 100644 --- a/sql/item_timefunc.cc +++ b/sql/item_timefunc.cc @@ -1642,32 +1633,10 @@ void Item_func_now::print(String *str, enum_query_type query_type) }
-int Item_func_now_local::save_in_field(Field *field, bool no_conversions) +bool Item_func_current_timestamp::val_native(THD *thd, Native *to) { - if (field->type() == MYSQL_TYPE_TIMESTAMP) - { - THD *thd= field->get_thd(); - my_time_t ts= thd->query_start(); - ulong sec_part= decimals ? thd->query_start_sec_part() : 0; - sec_part-= my_time_fraction_remainder(sec_part, decimals); - field->set_notnull(); - field->store_timestamp(ts, sec_part); - return 0; - } - else - return Item_datetimefunc::save_in_field(field, no_conversions); -} - - -/** - Converts current time in my_time_t to MYSQL_TIME representation for local - time zone. Defines time zone (local) used for whole NOW function. -*/ -void Item_func_now_local::store_now_in_TIME(THD *thd, MYSQL_TIME *now_time) -{ - thd->variables.time_zone->gmt_sec_to_TIME(now_time,
- set_sec_part(thd->query_start_sec_part(), now_time, this); - thd->used|= THD::TIME_ZONE_USED; + Timestamp ts(Timeval(thd->query_start(),
VCOL_TIME_FUNC); thd->query_start()); thd->query_start_sec_part()));
+ return null_value= ts.trunc(decimals).to_native(to, decimals);
can this possibly fail?
The is a small chance that to_native() can fail on EOM. I fixed Item_func_current_timestamp::val_native() and Item_func_sysdate_local::val_native() not to set null_value on errors (because they are NOT NULL) and added comments.
}
@@ -2732,7 +2693,6 @@ String *Item_func_tochar::val_str(String* str)
bool Item_func_from_unixtime::fix_length_and_dec(THD *thd) { - thd->used|= THD::TIME_ZONE_USED;
would be nice to have a test case for this. Like direct insert into a timestamp column no longer uses timezone - e.g. timestamp within dst switch time is inserted correctly.
and vice versa, when from_unixtime is converted to a string, like `insert ... values(concat(from_unixtime(...)))` then TIME_ZONE_USED is still set and show binlog events (or mysqlbinlog) should show that the timezone was correctly binlogged, as before.
Thanks, this is a good idea. I have added these tests: mysql-test/suite/binlog/t/type_datetime_func_current_timestamp.test mysql-test/suite/binlog/t/type_timestamp_func_current_timestamp.test
tz= thd->variables.time_zone; Type_std_attributes::set( Type_temporal_attributes_not_fixed_dec(MAX_DATETIME_WIDTH,
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik