Hi Alexey, Thanks for your review. On 11/26/2018 12:29 AM, Alexey Botchkov wrote:
Ok, fine with leaving sql_base_types.h and the MYSQL_TIME_STATUS as it is.
The last comment then: Item_func_trt_id::val_int() Item_interval_DDhhmmssff_typecast::val_str() there you get the 'current_thd' which is considered to be slow. Code looks like THD *thd= current_thd; Datetime::Options opt(TIME_CONV_NONE, thd); if (args[0]->get_date(thd, &commit_ts, opt)) I think it makes sence to add the Item_func_trt_id::m_opt and fill it in ::fix_fields().
Unfortunately this does not help much in these two places exactly. We need to pass thd to get_date() anyway. Methods like val_int(), val_str(), etc, should be fixed eventually to get an extra THD* parameter. But I checked the entiner patch again and fixed a few other places where some current_thd calls were redundant. Thanks!
Ok with me to push if you fix it like that.
One extra idea though. We can get rid of current_thd in set_date_length() too - add the THD * parameter there and call in in ::fix_fields() instead of ::fix_length_and_dec(). Since it doesn't seem critical, i'm fine if you leave it as it is :)
Alas, fix_length_and_dec() does not get THD* as an argument. Only fix_fields() does. fix_length_and_dec() should also be fixed to get THD*. Btw, perhaps 10.4 is a good timing for this.
Best regards. HF
On Sun, Nov 25, 2018 at 9:42 AM Alexander Barkov <bar@mariadb.com <mailto:bar@mariadb.com>> wrote:
Hi Alexey,
On 11/25/2018 09:05 AM, Alexander Barkov wrote: >> >> What do you think if we do this: >> - add THD* memver to the MYSQL_TIME_STATUS structure, >> so we can get rid of the THD * parameter to many functions? >> - send one MYSQL_TIME_STATUS* to the add_nanoseconds() >> instead of three {thd, &status->warnings, and status->nanoseconds} ? > > Changing all functions that accept both &warn and nsec to > accept a pointer to MYSQL_TIME_STATUS instead, like this: > > < xxxx(thd, &status->warnings, and status->nanoseconds); >> xxxx(thd, &status); > > is a very good idea. > > > I'd avoid mix "thd" to MYSQL_TIME_STATUS though. > It's a pure C structure. > > Does it sound OK? >
I tried this, but this does not save anything. See the attached patch.
git diff --stat sql/sql_time.cc | 6 +++--- sql/sql_type.cc | 2 +- sql/sql_type.h | 8 ++++++++ 3 files changed, 12 insertions(+), 4 deletions(-)
In all other places "*warn" and "nsec" come from different places (not from the same MYSQL_TIME_STATUS).
Btw, MYSQL_TIME_STATUS is actually an interface to low level pure C conversion functions like str_to_xxx() and number_to_xxx(). Let's not propagate it all around the code.
Can we leave the relevant code as is?
Thanks.