Fix for bug #76386: Prepared Statement formatter truncates fractional seconds from date/time column #3257

strongholdmedia · 2018-05-30T10:08:31Z

..that is also a duplicate of #67122.

URLs for bugs that have been fixed with this PR:
https://bugs.php.net/bug.php?id=76386
https://bugs.php.net/bug.php?id=76386

~~There is also a pull request #3245 that is intended to address this issue, but that is not acceptable since it ignores precision and only supports DATETIME(6) or TIMESTAMP(6).~~

This fix supports arbitrary precision from the driver.
I ran all previous tests, it does not break mysqli neither.

I tried to adapt to naming && coding conventions, unsure if this was successful.

..that is also a duplicate of #67122

antonkomarev · 2018-05-30T12:31:20Z

It should be targeted to php:PHP-7.1 branch and covered with tests.

strongholdmedia · 2018-05-30T18:55:36Z

I'd be glad to "cover it with tests", but facts are the following:

Since this is the driver, that is, a low-level component not directly accessible from "PHP userland", direct "phpt" tests are non-feasible
I am unsure if other types of tests are implemented as this component seems to have absolutely no tests at the moment to begin with.

My questions are:

Is there a C-level test framework aside the PHPish one mentioned above for certain components?
If the answer is "no", would anyone mind if I add tests to mysqli && PDO (as those are where issues
arising out of this one would surface)?

If anyone objects, and there is not a C level testing framework, then in order to create a separate "tests" folder here with the mysqlnd driver component, I'd need to duplicate quite a few files - like "connect.inc" or "skipif.inc" (that someone will possibly have to maintain in the future) to no apparent benefit, so that is possibly unwanted.

Please someone help me deciding with this.

nikic · 2018-05-30T19:26:00Z

It's fine to create tests in either mysqli or PDO mysql (or both). That's how mysqlnd is usually tested.

nikic · 2018-05-30T19:28:55Z

ext/mysqlnd/mysqlnd_ps_codec.c

-	length = mnd_sprintf(&value, 0, "%s%02u:%02u:%02u", (t.neg ? "-" : ""), t.hour, t.minute, t.second);
+    if (field->decimals > 0) {
+        char * format;
+        mnd_sprintf(&format, 0, "%%s%%02u:%%02u:%%02u.%%%02uu", field->decimals);


Instead of creating a dynamic format string, you should be able to use something like %0*u to specify the width dynamically.

Thanks.
Guess I could use some more libc knowledge. :)
Will do.

strongholdmedia · 2018-06-02T19:12:25Z

I added a test case - that covers all necessary changes - and also switched to the asterisk syntax.
I hope all tests in the tests folder will run and there is no need to "register" them somewhere.

weltling · 2018-06-04T09:51:25Z

ext/mysqli/tests/bug76386.phpt

+        'CREATE TABLE IF NOT EXISTS ts_test (id bigint unsigned auto_increment ' .
+        'primary key, ts timestamp default 0, ts2 timestamp(2) default 0, ' . 
+        'ts4 timestamp(4) default 0, ts6 timestamp(6) default 0) character ' .
+        'set utf8 collate utf8_general_ci;'


MySQL 5.7 might disallow zero for the default. IMO it were more robust not using a default value but inserting some explicit timestamp. As an option also using CURRENT_TIMESTAMP.

Thanks for that, I did not know that they have such plans.
The MySQL 5.7 that I use, as well as MariaDB 10.3, do not have such issues.
Updated the test according to your recommendations anyway.

weltling · 2018-06-04T10:11:13Z

ext/mysqlnd/mysqlnd_ps_codec.c


-	length = mnd_sprintf(&value, 0, "%s%02u:%02u:%02u", (t.neg ? "-" : ""), t.hour, t.minute, t.second);
+    if (field->decimals > 0) {
+        length = mnd_sprintf(&value, 0, "%s%02u:%02u:%02u.%0*u", (t.neg ? "-" : ""), t.hour, t.minute, t.second, field->decimals, t.second_part);


The second_part member is zend_ulong. It would be cleaner to use ZEND_ULONG_FMT_SPEC instead of u. Please see zend_long.h.

Of course, skipping that could cause trouble with e.g. future 16bit platforms.

After checking your recommendations in practice, I am fairly sure that using ZEND_ULONG_FMT_SPEC would not work.
What is needed here is to format the same ulong as an unsigned zero-padded decimal having any number of digits between 1-6.
While ZEND_ULONG_FMT_SPEC resolves - at least here - to PRIu64, we do not want an unsigned long here but a predetermined amount of digits, zero padded.
..um, excuse me, the new test (or, rather, the new defaults) fail in the same way when I used ZEND_ULONG_FMT_SPEC.
Let me check what else we have here.

zend_ulong is dynamic, it's 64- or 32-bit depending on the platform used. Using the macro is trivial. 16-bit platforms are not supported at all, so nothing to care. Principally, one could also leave u as the fractional part won't be that big anyway. It's just a formal thing - the format to match the datatype. It doesn't affect the field width, as the decimal places number is still being passed. I'm going to check once more tomorrow.

Thanks.

Okay, after recompiling PHP with --enable-debug, and using mysql_debug, and adding some extra DBG_INF_FMT macros, it became apparent that while the decimals member is always right (2, 4, 6, the precision that is configured), the second_part member has its value always set to microseconds internally in mysql.

Sigh.

As such, I think we are not heading towards the long format, since we want to get "77" from "770000" with a decimals value of 2, and even "9" from a long value of "900000" with a decimals value of 1.

This also means the "second_part" member name is a misnomer, since it is always microseconds, no matter the actual precision configured.

weltling · 2018-06-04T10:12:08Z

ext/mysqli/tests/bug76386.phpt

+    $stmt->fetch();
+    var_dump($ts, $ts2, $ts4, $ts6);
+} finally {
+    $link->query('DROP TABLE IF EXISTS ts_test;');


It would make sense to drop that table right before creating it. Otherwise, the --CLEAN-- section should be used for the cleanup.

strongholdmedia · 2018-06-04T22:00:07Z

This is what I came up with.
I am fairly sure that you will be even more reluctant to merge this now, since it introduces a "math" library dependency, and whatnot, but this is an actually working solution with its complexities..
Thanks for the previous ideas, without them it hadn't been possible to actually find the underlying issue.
I am open to further ones of course.

imbrish · 2018-06-05T08:51:25Z

ext/mysqlnd/mysqlnd_ps_codec.c

 	}

-	length = mnd_sprintf(&value, 0, "%s%02u:%02u:%02u", (t.neg ? "-" : ""), t.hour, t.minute, t.second);
+    if (field->decimals > 0 && field->decimals < 7) {


Why ignoring fraction with higher precision?

Currently it is declared that the precision is microseconds at most; according to the manual, higher precision is not possible.
This is only to safeguard against possible future hard-to-find issues should any behavioral changes go unnoticed.

imbrish · 2018-06-05T09:00:44Z

ext/mysqlnd/mysqlnd_ps_codec.c

+            t.minute,
+            t.second,
+            field->decimals,
+           (int) (t.second_part / pow(10, 6 - field->decimals))


This doesn't seem right, what was wrong with just left padding it up to the number of decimals?

If the passed field width is smaller than the number of digits, all the digits would be included.

I was concerned about a field the same size as the number of digits, but smaller than 6. Consider TIME(3) with value of 123 - wouldn't that become 123 / pow(10, 3) = 123 / 1000 ~= 0? Or am I missing sth and second_part always comes with a fixed number of digits?

Seems it always delivers 6 digits, the irrelevant places are filled with zeroes. Like with TIMESTAMP(3), a number would be like 123000. At least those DB servers I was able to test do that.

That is exactly the case.

weltling · 2018-06-05T09:42:05Z

@strongholdmedia thanks for checking the comments. As for me, the trailing zeroes should be cut off. Another option to do that would be to put t.second_part into a temporary buffer and then use it as a string. Something like this quick snippet

char _tmp[MAX_LENGTH_OF_LONG];
snprintf(_tmp, MAX_LENGTH_OF_LONG, ZEND_ULONG_FMT, t.second_part);
_tmp[field->decimals] = '\0';

Then append _tmp with %s. One or another effort is needed anyway, to remove the trailing zeroes.

The MySQL manual says

A DATETIME or TIMESTAMP value can include a trailing fractional seconds part in up to microseconds (6 digits) precision.

So it might be ok to limit the fractional digits hardcoded to 6. Not sure we have this information already somewhere in the driver. It might be not future proof, however it's unlikely that limit to change in the near future, if at all. MySQL 8.0 documents it that way, too. Perhaps more thoughts are needed about this point.

The other place with ps_fetch_time should probably be covered by a test as well.

I would rather suggest to be reluctant to target stable branches, that's it. It is of course a bug fix, but the change of the delivered date format might be sudden for some apps, fe when the timestamp columns was declared with the precision already. Thus, IMHO it would be better to get it stable and see if it's in time for 7.3, otherwise to go by 7.4.

Thanks.

weltling · 2018-06-05T09:43:16Z

ext/mysqlnd/mysqlnd_ps_codec.c

+            t.minute,
+            t.second,
+            field->decimals,
+           (int) (t.second_part / pow(10, 6 - field->decimals))


For int the placeholder would be %d ;)

Thanks. :D Sure enough. I should cast to UINT32_T, given that it can only take a value between 0-999999.
But that is theoretical only, as now I have the problem that my last commit failed the test, whilst it ran without failure or being skipped locally.
Apart from versions, etc, the only obvious difference is that I use "127.0.0.1" as MYSQL_TEST_HOST without using mysqld.sock (that seems to be the default for "localhost" for years now).

As for the target branch.. most people (like me) are forced to use emulated_prepares with PDO (that is on by default anyway, but many of us would have reasons to turn it off).
Now having a delta of 1 second was totally acceptable like, um, 10 years ago, but in a HA system, many things could and should happen in one second.
There are some bug reports for stuff like PHP frameworks and so on, with people having no idea what and why is this happening.

The support for actually working microseconds is in since MySQL 5.6.4 (milestone 7, released on 2011-12-20; stable as 5.6.10 since 2013-02-05; neither seems yesterday).

weltling · 2018-06-05T10:16:54Z

ext/mysqlnd/mysqlnd_ps_codec.c

+        length = mnd_sprintf(
+            &value,
+            0,
+            "%s%02u:%02u:%02u.%0*" ZEND_ULONG_FMT_SPEC,


The field width is not needed anymore, as the argument was truncated.

Am I missing something? I thought it still needed to be zero-padded when smaller than 10^(decimals-1).
The leading zero though seems superfluous indeed.

strongholdmedia · 2018-06-05T20:46:11Z

If the test fails again on the Travis instance, may I be able to obtain a dockerfile or such to reproduce the exact build environment to debug the issue further please.

strongholdmedia · 2018-06-05T21:17:38Z

Now "my" build failed with

FAILED TEST SUMMARY
---------------------------------------------------------------------
socket_export_stream: Bad arguments [C:\projects\php-src\ext\sockets\tests\socket_export_stream-2.phpt]
socket_import_stream: Bad arguments [C:\projects\php-src\ext\sockets\tests\socket_import_stream-2.phpt]
=====================================================================

This CI stuff doesn't seem very robust, now does it.

weltling · 2018-06-06T09:28:23Z

ext/mysqlnd/mysqlnd_ps_codec.c

+        length = mnd_sprintf(
+            &value,
+            0,
+            "%s%02u:%02u:%02u.%*u",


The asterisk is also unnecessary.

Sorry but you seem to have missed my previous reply.
When you have 9 as a long, with a decimals value of 3, that is represented as a decimal 9000 (microseconds), but you still want to return 009, not 9.
That being said, I am unsure as of what to add in the place of asterisks if I remove them.

You're right. Forgot about the opposite case. Then it looks like it's missing a zero, should be %0*u so a small number is padded on the left. Please lets test this case by inserting the appropriate data, too.

weltling · 2018-06-06T09:34:27Z

Yeah, those two are sporadic and thus unrelated. To the actual failure is shown iin the job log https://travis-ci.org/php/php-src/jobs/388499682#L2072 . It could be same about the strictness mode, like it was before about the default zero timestamp. It would help to add some error checks, so they're shown in the log output as well. Most likely it's a test issue. Either the table creation or the insert fails. Separating into two different files would also help to isolate, as the previous test didn't fail.

Thanks.

weltling · 2018-06-07T08:01:26Z

ext/mysqli/tests/bug76386.phpt

+$link->query('DROP TABLE IF EXISTS t_test;');
+$link->query(
+    'CREATE TABLE t_test (id bigint unsigned auto_increment primary key, ' .
+    't time default current_time(), t2 time(2) default current_time(2), t4 ' .


I guess default current_time is what it doesn't like. It only initalizes timestamp/datetime automatically.

Ah, surely enough. I am a total idiot and thought that was IMPOSSIBLE.
It is just that I am using MariaDB, that DOES support this, so I assumed MySQL does as well.
It is the second time that I run into exactly this kind of fail recently.
Anyway that's why I asked about the "official" dockerfile earlier.

strongholdmedia · 2018-06-07T19:40:18Z

So to add, MariaDB 10.3 does support "CURRENT_TIME(n)" as initializer, while MySQL, at least the version used in the CI environment, doesn't.
That's why the test ran great for me while it failed repetitively in the CI environment.
(I could still use a dockerfile or detailed environment building instructions.)

weltling · 2018-06-08T13:06:06Z

The current solution seems fine, as for me. Lets see, if there are further comments. It should suit for 7.3 alpha, if everything goes ok.

Thanks.

krakjoe · 2018-06-14T09:19:45Z

/cc @cmb69

cmb69 · 2018-06-14T11:38:56Z

I can't comment on technical details here, but considering that this apparently would fix a bug, it's fine for me to put it into 7.3.

weltling · 2018-06-14T12:48:58Z

@strongholdmedia looks like it's fine to merge. Could you add an UPGRADING note please?

Thanks.

strongholdmedia · 2018-06-14T19:59:10Z

@weltling sure enough, I did.
I split the note into two, with some redundancy for the sake of easier navigation.
If brevity is preferred instead (or there is anything else), please let me know.

weltling · 2018-06-15T05:38:48Z

Merged as 71c0432 and also added NEWS. Thanks for all your work on this!

antonkomarev · 2018-06-15T07:28:51Z

Thank you for the great job guys!

Attempt at fixing PHP bug #76386

4e741aa

..that is also a duplicate of #67122

strongholdmedia mentioned this pull request May 30, 2018

Fix #67122: PDO::ATTR_EMULATE_PREPARES = false drops microseconds from timestamp #3245

Closed

nikic reviewed May 30, 2018

View reviewed changes

Added test case and switched to printf dynamic length specifier

4e4b9a2

weltling suggested changes Jun 4, 2018

View reviewed changes

strongholdmedia added 2 commits June 4, 2018 21:27

Made the test more concise

972ef3d

Solved the actual problem instead of a superficial imaginary one

75f57f7

imbrish reviewed Jun 5, 2018

View reviewed changes

weltling reviewed Jun 5, 2018

View reviewed changes

weltling approved these changes Jun 5, 2018

View reviewed changes

weltling reviewed Jun 5, 2018

View reviewed changes

Revert ZEND_ULONG_FMT_SPEC

19f2ed5

strongholdmedia added 2 commits June 5, 2018 22:53

Casted milliseconds to uint32_t on any platform

ff01565

Extended test case to TIME-typed columns

0c840f8

Made test more robust by not reusing same identifier

21d3aab

weltling reviewed Jun 6, 2018

View reviewed changes

strongholdmedia added 2 commits June 6, 2018 22:14

Split test into two totally separate pars to avoid random issues

f6a0d4e

Added displaying debug information in test if prepare fails

19f54ae

weltling reviewed Jun 7, 2018

View reviewed changes

Reverted to proper padding syntax and fixed test

d82a8ba

krakjoe added Bug Waiting on RM labels Jun 14, 2018

antonkomarev mentioned this pull request Jun 14, 2018

DateTime Precision is lost when adding date to model factories laravel/framework#24214

Closed

Added UPGRADING note for the fix of bug #76386

d3054a7

weltling closed this Jun 15, 2018

strongholdmedia mentioned this pull request Sep 27, 2019

Cast client ids to strings before comparing them thephpleague/oauth2-server#1054

Closed

Fix for bug #76386: Prepared Statement formatter truncates fractional seconds from date/time column #3257

Fix for bug #76386: Prepared Statement formatter truncates fractional seconds from date/time column #3257

Uh oh!

Conversation

strongholdmedia commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonkomarev commented May 30, 2018

Uh oh!

strongholdmedia commented May 30, 2018

Uh oh!

nikic commented May 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strongholdmedia commented Jun 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strongholdmedia Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strongholdmedia commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weltling commented Jun 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strongholdmedia commented Jun 5, 2018

Uh oh!

strongholdmedia commented Jun 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weltling Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

strongholdmedia commented May 30, 2018 •

edited

Loading

strongholdmedia Jun 4, 2018 •

edited

Loading

weltling Jun 7, 2018 •

edited

Loading

krakjoe commented Jun 14, 2018 •

edited

Loading

antonkomarev commented Jun 15, 2018 •

edited

Loading