Re: gettimeofday cause crash on Windows

On Wed, Feb 11, 2015 at 9:11 PM, Asif Naeem <anaeem(dot)it(at)gmail(dot)com> wrote:
>
> Hi,
>
> PG95/pg_test_fsync utility (MSVC built) is crashing on Windows and It is caused by gettimeofday (src\port\gettimeofday.c) function. The following check-in raised this issue i.e.
>
>> commit 8001fe67a3d66c95861ce1f7075ef03953670d13
>> Author: Simon Riggs <simon(at)2ndQuadrant(dot)com>
>> Date: Mon Dec 8 23:36:06 2014 +0900
>> Windows: use GetSystemTimePreciseAsFileTime if available
>> PostgreSQL on Windows 8 or Windows Server 2012 will now
>> get high-resolution timestamps by dynamically loading the
>> GetSystemTimePreciseAsFileTime function. It'll fall back to
>> to GetSystemTimeAsFileTime if the higher precision variant
>> isn't found, so the same binaries without problems on older
>> Windows releases.
>> No attempt is made to detect the Windows version. Only the
>> presence or absence of the desired function is considered.
>> Craig Ringer
>
>
> It make PG gettimeofday() implementation depend on init_win32_gettimeofday() function call, otherwise gettimeofday() function is going to crash the related process because of NULL pg_get_system_time function pointer. PFA patch gettimeofday_win32_v1.patch, it enables necessary initialization in related utilities.
>
> PFA patch gettimeofday_win32_suggestion_v1.patch, it suggest minor change to make it more versatile version of PG gettimeofday() function. It makes it more crash safe and by default it uses much faster or lesser CPU intensive GetSystemTimeAsFileTime function. If a pg utility do require high precision, it can call init_win32_precise_gettimeofday() to try to elevate the precision when available. Please do let me know if I missed something or more information is required.

I didn't follow much the thread implementing this feature, but this
commit has somewhat not taken into account the fact that the contents
of src/port could as well be used for the frontends, per se the
comments on top of init_win32_gettimeofday mentioning only the backend
startup, something not wrong in itself, but not completely right
either. So what we have now is an unpleasant crash trigger for any
client applications on Windows < 2k12 linking to libpqport that call
gettimeofday(). I would not be surprised that we would get bug reports
of the type "why my client binary crashes suddendly with 9.5" if we
keep the code as-is.

So ISTM that your patch does the correct thing by setting
pg_get_system_time to &GetSystemTimeAsFileTime to not break any
existing applications, and that we should as well update any in-core
binary tools to switch to the precise API. I rewrote your first patch
as the attached, which is really needed to fix the potential crash
problems with some comment cleanup, in addition to a second patch that
switches the frontend binaries to use the precise routine. After a
quick scan of the code it seems that you have forgotten nothing, we
need to switch correctly to the precise API for pg_test_fsync,
pg_resetxlog and pg_basebackup. If anything else is forgotten, we
could always add it later, for now any existing frontend binaries
would not crash.

It will be important to mention as well in the release notes how
client binaries can update to the precise API as well.
Regards,
--
Michael