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 <b...@mariadb.com > <mailto:b...@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. > _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
