maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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@xxxxxxxxxxx
> <mailto:bar@xxxxxxxxxxx>> 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.
>