Re: [PATCH] Remove unncessary localtime() calls during data type conversion

On 06/14/2015 06:36 AM, Michael Paquier wrote:
> On Sat, Jun 13, 2015 at 9:42 AM, Nikhil Deshpande <ndeshpande@vmware.com> wrote:
>> While doing some profiling work on an ODBC client, we noticed tons
>> of libc localtime() and __tz_convert() calls on a hot code path
>> (SQLFetch()), but most of the fields being retrieved were not of
>> date/time type.
>>
>>
>> These libc calls are costly (non-trivial work in terms of string ops,
>> timzone conversion etc.) and the cost adds up quickly for high-frequency
>> call like SQLFetch(). We noticed that this penalty is paid even
>> for data types which aren't date/time related. Code inspection
>> showed localtime() call in convert.c:copy_and_convert_field()
>> at function start, being invoked regardless of whether output of
>> localtime() is used or not.
>>
>> Attached is a non-intrusive patch that moves the call down
>> to where it's needed (for both getting data from server and
>> sending data to server), but I'm not sure if the patch is complete
>> (it covers the case where source data type is time but
>> destination needs date part, "TIME->DATE[TIME]").
>>
>> Could you please review this patch and modify it for correctness
>> and consider for committing it?
>
> Indeed. I would move as well the declarations of tim more within the
> inner loops.

This changes behaviour, when converting from text to date column.
Without this patch, if the string you're converting from is missing a
year/month/date field, it will be filled with the current date. This
modification to the result-conversions test case shows the difference:

--- a/test/src/result-conversions-test.c
+++ b/test/src/result-conversions-test.c
@@ -626,6 +626,8 @@ int main(int argc, char **argv)
      test_conversion("float8", "Infinity", SQL_C_DOUBLE, "SQL_C_DOUBLE", 20);
      test_conversion("float8", "-Infinity", SQL_C_DOUBLE, "SQL_C_DOUBLE", 20);

+    test_conversion("text", "", SQL_C_TYPE_DATE, "SQL_C_TYPE_DATE", 20);
+
      /* Clean up */
      test_disconnect();


We don't currently contain tests like that in the test suite, as
mentioned in the comment earlier in result-conversions-test.c:

>     /*
>      * Converting arbitrary things to date and timestamp produces results that
>      * depend on the current timestamp, because the driver substitutes the
>      * current year/month/datefor missing values. Disable for now, to get a
>      * reproducible result.
>      */

Would be nice to include those tests, perhaps by printing the difference
to current date instead of the absolute date.

- Heikki