Ido*sqlConnection#FieldToEscapedString(): don't write out of range time #10058
Conversation
|
PING @sciolto |
|
Al2Klimov
force-pushed
the
error-timestamp-out-of-range-53323
branch
from
a98105b to
dcf4955
Compare
Al2Klimov
force-pushed
the
error-timestamp-out-of-range-53323
branch
from
dcf4955 to
34ced89
Compare
Al2Klimov
changed the title
Al2Klimov
force-pushed
the
error-timestamp-out-of-range-53323
branch
from
34ced89 to
f2f6387
Compare
MySQL's FROM_UNIXTIME() NULLs ts <1970, errors for >2038. Postgres' TO_TIMESTAMP() errors for all ts not between 4713BC - 294276AD.
Al2Klimov
force-pushed
the
error-timestamp-out-of-range-53323
branch
from
f2f6387 to
c6f9de5
Compare
| std::ostringstream msgbuf; | ||
| msgbuf << "FROM_UNIXTIME(" << ts << ")"; | ||
| msgbuf << "FROM_UNIXTIME(" << std::fixed << std::setprecision(0) | ||
| << std::fmin(std::fmax(ts, 1.0), 2147483647.0) << ")"; |
There was a problem hiding this comment.
Wouldn't using std::clamp make this more readable? Same goes for the other change at PostgreSQL?
There was a problem hiding this comment.
Only if we all agree not to backport this to v2.13 which doesn't support C++17.
There was a problem hiding this comment.
Seriously, std::clamp shouldn't be a deal breaker for not back porting it. So, to me this should also be fine!
There was a problem hiding this comment.
Wouldn't using
std::clampmake this more readable?
Depends, my understand was that fmin/fmax was used instead of the regular min/max for handling the dangers of floating point (otherwise, NaN might give you undefined behavior, C++ is fun).
Only if we all agree not to backport this to v2.13 which doesn't support C++17.
General remark on that: one option for such situations would be to make a dedicated commit for adapting the code to make use of newer C++ features that could then just be omitted when backporting.
|
To say it with a meme: it "always has been". I mean, if MySQL itself supported ts>2038, we didn't have to limit it to 2038 in this PR. |
Why do you think it's necessary to include this in our documentation? After all, this limit isn't specific to IDO only but the DBMs itself, so doesn’t make sense to me to document such a constraint here. @Al2Klimov please rebase! |
|
Rebase? Does everything else LGTY? |
Shouldn’t this be a sufficient response to your question? #10058 (review) |
|
No, because @julianbrost seems to still want to review it. Also, the last time IIRC, you also explicitly allowed to force merge – that's what I'd do. |
MySQL's FROM_UNIXTIME() NULLs ts <1970, errors for >2038. Postgres' TO_TIMESTAMP() errors for all ts not between 4713BC - 294276AD.
ref/IP/53323