Bug #101457 Optimizer trace still prints floating point numbers imprecisely
Submitted: 4 Nov 2020 11:42 Modified: 12 Nov 2020 22:16
Reporter: Alexey Kopytov Email Updates:
Status: Closed Impact on me:
None 
Category:MySQL Server: Optimizer Severity:S3 (Non-critical)
Version:8.0 OS:Any
Assigned to: CPU Architecture:Any

[4 Nov 2020 11:42] Alexey Kopytov
Description:
Reporting this as follow-up to bug#94672.

Even though the relevant code has been changed to use my_gcvt() in commit
ab4919602ffe5543b4ebbab4d79c008c023be1c6 fixing bug #94672, I think
my_gcvt() was used in a wrong way which may lead to precision issues.

This code in sql/opt_trace.cc (8.0.22):

```
Opt_trace_struct &Opt_trace_struct::do_add(const char *key, double val) {
  DBUG_ASSERT(started);
  char buf[32];  // 32 is enough for digits of a double
  /*
    To fit in FLT_DIG digits, my_gcvt rounds DBL_MAX (1.7976931...e308), or
    anything >=1.5e308, to 2e308. But JSON parsers refuse to read 2e308. So,
    lower the number.
  */
  my_gcvt(std::min(1e308, val), MY_GCVT_ARG_DOUBLE, FLT_DIG, buf, nullptr);
  DBUG_PRINT("opt", ("%s: %s", key, buf));
  stmt->add(key, buf, strlen(buf), false, false);
  return *this;
}
```

There is a number of things that look wrong in that code:

- FLT_DIG does not make any sense as the `width` argument to
  my_gcvt(). And neither does DBL_DIG. Those constants define some
  limits in terms of decimal digits. The `width` argument to my_gcvt()
  is, well, the field width in characters.

- the real field width in the case is the size of stack-allocated `buf`

- the size of buf (32 bytes) looks arbitrary. m_string.h defines
  MY_GCVT_MAX_FIELD_WIDTH. That is what should have been used here.

- with everything above implemented, the workaround with limiting the
  printed number to 1e308 will become pointless along with the
  corresponding comment.

The only problem that needs extra work here is the RapidJSON bug with
DBL_MAX: https://github.com/Tencent/rapidjson/issues/710. Apparently,
RapidJSON refuses to parse 1.7976931348623157E+308, which is a perfectly
valid IEEE-754 number. I'm not sure how important it is to provide
workarounds for bugs in third-party software. If that's absolutely
necessary, it would be more correct to check the input for DBL_MAX
specifically (rather than limiting it to some arbitrary constant like
1e308) and printing something as close to it as possible that would be
parsed correctly by RapidJSON.

How to repeat:
Look at the code in Opt_trace_struct &Opt_trace_struct::do_add(const char *key, double val).
[4 Nov 2020 13:49] MySQL Verification Team
Hi Kaamos,

We completely concur with your analysis.

Verified as reported.
[12 Nov 2020 22:16] Jon Stephens
Documented fix as follows in the MySQL 8.0.24 changelog:

    The optimizer trace printed floating-point numbers with a
    maximum six characters, which meant that the precision could be
    very low for many values, given that sign, decimal point, and
    exponent could use many of these characters. This was especially
    problematic for large numbers, whose precision could thus be as
    small as 1, and which could be rounded to values whose absolute 
    value exceeded DBL_MAX and so could be rejected by JSON parsers.

    Now such numbers are always printed in the optimizer trace with
    a precision of 6.

Closed.
[13 Nov 2020 12:45] MySQL Verification Team
Thanks, Jon.