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

Hi Michael,

Sorry for the late reply.

On 2016/04/25 15:30, Michael Paquier wrote:
> On Tue, Sep 1, 2015 at 6:11 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>> 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.
> Waking up an old thread, after being reminded by some colleagues that
> there are no improvements in the upstream code yet regarding that...
>
>> 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:
> Yes, I can see the difference. I am noticing that only SQL_C_TYPE_DATE
> and SQL_C_TYPE_TIMESTAMP are actually converting the non-defined
> fields using the localtime values. SQL_C_TYPE_TIME is not impacted at
> all. Is that expected?
>
>>> /*
>>> * 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.
> Yeah, I have done that in the attached with 0001, however I don't
> really see how we can actually remove those comments, because some of
> the tests include a fixed timestamp, and need to use a fixed
> timestamp.
>
> I am attaching patch 0002 that is a rebase of the main patch for HEAD,
> that introduces no regressions based on the tests I have added in
> 0001. 0002 is proving to improve performance of the ODBC driver by
> more or less 20% by reducing those localtime() calls in perf specs for
> an application that is pretty hot with SQLFetch. That's quite
> something.

I would take care of the patches.
Thanks.

regards,
Hiroshi Inoue