Thread: gettimeofday cause crash on Windows

On Wed, Feb 11, 2015 at 9:11 PM, Asif Naeem <anaeem.it@gmail.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@2ndQuadrant.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()
functionis 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
GetSystemTimeAsFileTimefunction. 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
somethingor 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

On 2015-02-12 04:48:00 -0800, Michael Paquier wrote:
> 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 think I'd rather have it crash so people are forced to adjust the
client applications. The number of things linking to pgport should be
rather low.

Greetings,

Andres Freund

On Thu, Feb 12, 2015 at 9:54 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2015-02-12 04:48:00 -0800, Michael Paquier wrote:
>> 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 think I'd rather have it crash so people are forced to adjust the
> client applications. The number of things linking to pgport should be
> rather low.

I'd take it on the safe side, not the hard one :)

FWIW, applications are not broken if they use GetSystemTimeAsFileTime,
simply less precise. So we should push for improvements, not mandatory
moves without a deprecation period.
--
Michael

On 2015-02-12 22:09:43 +0900, Michael Paquier wrote:
> On Thu, Feb 12, 2015 at 9:54 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2015-02-12 04:48:00 -0800, Michael Paquier wrote:
> >> 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 think I'd rather have it crash so people are forced to adjust the
> > client applications. The number of things linking to pgport should be
> > rather low.
>
> I'd take it on the safe side, not the hard one :)

Meh. If this were libpq, I'd tend to agree, but it isn't.

> FWIW, applications are not broken if they use GetSystemTimeAsFileTime,
> simply less precise. So we should push for improvements, not mandatory
> moves without a deprecation period.

pgport isn't an actual enduser library. And we feel free to yank stuff
in/out of it all the time.

Greetings,

Andres Freund

--
 Andres Freund                       http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Don=E2=80=99t you feel gettimeofday() implementation is incomplete in curre=
nt
state, Is there any other example of function port in PG that could lead to
crash if its related init_* function is not called earlier ?. As mentioned
by Michael, it seems not enough documented (release notes etc), at least it
need to give warning etc instead of simply crash the related process.
GetSystemTimeAsFileTime() function is less precise but it is quite fast and
less CPU intensive then GetSystemTimePreciseAsFileTime() function,
GetSystemTimePreciseAsFileTime() can effect performance of an application.
With the proposed patch, gettimeofday() function is more safe and more
versatile, a contrib may use it as per its need of precision. Thanks.

Regards,
Muhammad Asif Naeem


On Thu, Feb 12, 2015 at 6:13 PM, Andres Freund <andres@2ndquadrant.com>
wrote:

> On 2015-02-12 22:09:43 +0900, Michael Paquier wrote:
> > On Thu, Feb 12, 2015 at 9:54 PM, Andres Freund <andres@2ndquadrant.com>
> wrote:
> > > On 2015-02-12 04:48:00 -0800, Michael Paquier wrote:
> > >> I didn't follow much the thread implementing this feature, but this
> > >> commit has somewhat not taken into account the fact that the content=
s
> > >> of src/port could as well be used for the frontends, per se the
> > >> comments on top of init_win32_gettimeofday mentioning only the backe=
nd
> > >> 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 repor=
ts
> > >> 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 think I'd rather have it crash so people are forced to adjust the
> > > client applications. The number of things linking to pgport should be
> > > rather low.
> >
> > I'd take it on the safe side, not the hard one :)
>
> Meh. If this were libpq, I'd tend to agree, but it isn't.
>
> > FWIW, applications are not broken if they use GetSystemTimeAsFileTime,
> > simply less precise. So we should push for improvements, not mandatory
> > moves without a deprecation period.
>
> pgport isn't an actual enduser library. And we feel free to yank stuff
> in/out of it all the time.
>
> Greetings,
>
> Andres Freund
>
> --
>  Andres Freund                     http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>

Asif Naeem wrote:
> Don’t you feel gettimeofday() implementation is incomplete in current
> state, Is there any other example of function port in PG that could lead to
> crash if its related init_* function is not called earlier ?. As mentioned
> by Michael, it seems not enough documented (release notes etc), at least it
> need to give warning etc instead of simply crash the related process.
> GetSystemTimeAsFileTime() function is less precise but it is quite fast and
> less CPU intensive then GetSystemTimePreciseAsFileTime() function,
> GetSystemTimePreciseAsFileTime() can effect performance of an application.
> With the proposed patch, gettimeofday() function is more safe and more
> versatile, a contrib may use it as per its need of precision. Thanks.

Ideally we would the Precise dance automatically, without requiring the
program to call an initialization function.  Otherwise, programs may
never be updated to use Precise rather than the stock function.  I guess
this is the reason Andres prefers a crash: to force programs to be
updated to include the initialization step.

I wonder if it would work to set the function initially to the
initialization function, so that on the first call the Precise version,
if it exists, is detected (or the stock version otherwise) and set as
the function to call for later -- without requiring a jump in every
subsequent execution of gettimeofday(), of course.  The initialization
function itself would have to return the value returned by
GetSystemTimePreciseAsFileTime, of course.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I wonder if it would work to set the function initially to the
> initialization function, so that on the first call the Precise version,
> if it exists, is detected (or the stock version otherwise) and set as
> the function to call for later -- without requiring a jump in every
> subsequent execution of gettimeofday(), of course.  The initialization
> function itself would have to return the value returned by
> GetSystemTimePreciseAsFileTime, of course.

+1.  The idea that we are going to alter the calling requirements around a
POSIX-standard function is utterly horrible.  It will break third-party
code, very probably.

            regards, tom lane

On 2015-02-12 12:14:03 -0300, Alvaro Herrera wrote:
> Asif Naeem wrote:
> > Don’t you feel gettimeofday() implementation is incomplete in current
> > state, Is there any other example of function port in PG that could lead to
> > crash if its related init_* function is not called earlier ?. As mentioned
> > by Michael, it seems not enough documented (release notes etc), at least it
> > need to give warning etc instead of simply crash the related process.
> > GetSystemTimeAsFileTime() function is less precise but it is quite fast and
> > less CPU intensive then GetSystemTimePreciseAsFileTime() function,
> > GetSystemTimePreciseAsFileTime() can effect performance of an application.
> > With the proposed patch, gettimeofday() function is more safe and more
> > versatile, a contrib may use it as per its need of precision. Thanks.
>
> Ideally we would the Precise dance automatically, without requiring the
> program to call an initialization function.  Otherwise, programs may
> never be updated to use Precise rather than the stock function.  I guess
> this is the reason Andres prefers a crash: to force programs to be
> updated to include the initialization step.
>
> I wonder if it would work to set the function initially to the
> initialization function, so that on the first call the Precise version,
> if it exists, is detected (or the stock version otherwise) and set as
> the function to call for later -- without requiring a jump in every
> subsequent execution of gettimeofday(), of course.  The initialization
> function itself would have to return the value returned by
> GetSystemTimePreciseAsFileTime, of course.

Yes, that sounds like a good idea.

Greetings,

Andres Freund

--
 Andres Freund                       http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Feb 13, 2015 at 4:41 AM, Asif Naeem wrote:
> On Thu, Feb 12, 2015 at 10:53 PM, Andres Freund wrote:
>>
>> On 2015-02-12 12:14:03 -0300, Alvaro Herrera wrote:
>> > Asif Naeem wrote:
>> > > Don't you feel gettimeofday() implementation is incomplete in current
>> > > state, Is there any other example of function port in PG that could
>> > > lead to
>> > > crash if its related init_* function is not called earlier ?. As
>> > > mentioned
>> > > by Michael, it seems not enough documented (release notes etc), at
>> > > least it
>> > > need to give warning etc instead of simply crash the related process.
>> > > GetSystemTimeAsFileTime() function is less precise but it is quite
>> > > fast and
>> > > less CPU intensive then GetSystemTimePreciseAsFileTime() function,
>> > > GetSystemTimePreciseAsFileTime() can effect performance of an
>> > > application.
>> > > With the proposed patch, gettimeofday() function is more safe and more
>> > > versatile, a contrib may use it as per its need of precision. Thanks.
>> >
>> > Ideally we would the Precise dance automatically, without requiring the
>> > program to call an initialization function.  Otherwise, programs may
>> > never be updated to use Precise rather than the stock function.  I guess
>> > this is the reason Andres prefers a crash: to force programs to be
>> > updated to include the initialization step.
>> >
>> > I wonder if it would work to set the function initially to the
>> > initialization function, so that on the first call the Precise version,
>> > if it exists, is detected (or the stock version otherwise) and set as
>> > the function to call for later -- without requiring a jump in every
>> > subsequent execution of gettimeofday(), of course.  The initialization
>> > function itself would have to return the value returned by
>> > GetSystemTimePreciseAsFileTime, of course.
>>
>> Yes, that sounds like a good idea.

+1. Excellent idea.

> Superb idea. PFA patch. Please do let me know if I missed something. Thanks.

I looked at your patch and tested it with msvc and it looks good to me.

Note not related to this patch at all: shouldn't we update the
copyright notice on top of this gettimeofday.c btw with some PGDG
stuff?
--
Michael

On Thu, Feb 12, 2015 at 9:59 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> I looked at your patch and tested it with msvc and it looks good to me.

Committed with cosmetic adjustments.  Hopefully one of you fine
gentlemen will jump in to help if this turns out to break, because I
don't do Windows.

> Note not related to this patch at all: shouldn't we update the
> copyright notice on top of this gettimeofday.c btw with some PGDG
> stuff?

I think so.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Feb 21, 2015 at 12:20 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Feb 12, 2015 at 9:59 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> I looked at your patch and tested it with msvc and it looks good to me.
>
> Committed with cosmetic adjustments.  Hopefully one of you fine
> gentlemen will jump in to help if this turns out to break, because I
> don't do Windows.

currawong doesn't like this:

"c:\prog\bf\root\HEAD\pgsql.build\pgsql.sln" (default target) (1) ->
(postgres target) ->
  .\src\port\gettimeofday.c(53): error C2440: 'initializing' : cannot
convert from 'void (__cdecl *)(LPFILETIME)' to 'PgGetSystemTimeFn'


"c:\prog\bf\root\HEAD\pgsql.build\pgsql.sln" (default target) (1) ->
(misc\libpgport target) ->
  .\src\port\gettimeofday.c(53): error C2440: 'initializing' : cannot
convert from 'void (__cdecl *)(LPFILETIME)' to 'PgGetSystemTimeFn'

    0 Warning(s)
    2 Error(s)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Feb 21, 2015 at 1:00 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Feb 21, 2015 at 12:20 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Thu, Feb 12, 2015 at 9:59 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> I looked at your patch and tested it with msvc and it looks good to me.
>>
>> Committed with cosmetic adjustments.  Hopefully one of you fine
>> gentlemen will jump in to help if this turns out to break, because I
>> don't do Windows.
>
> currawong doesn't like this:
>
> "c:\prog\bf\root\HEAD\pgsql.build\pgsql.sln" (default target) (1) ->
> (postgres target) ->
>   .\src\port\gettimeofday.c(53): error C2440: 'initializing' : cannot
> convert from 'void (__cdecl *)(LPFILETIME)' to 'PgGetSystemTimeFn'
>
>
> "c:\prog\bf\root\HEAD\pgsql.build\pgsql.sln" (default target) (1) ->
> (misc\libpgport target) ->
>   .\src\port\gettimeofday.c(53): error C2440: 'initializing' : cannot
> convert from 'void (__cdecl *)(LPFILETIME)' to 'PgGetSystemTimeFn'
>
>     0 Warning(s)
>     2 Error(s)

I assume that the problem here must be whatever secret sauce WINAPI
pours on the declaration.  PgSystemTimeFn is declared like this:

typedef VOID (WINAPI *PgGetSystemTimeFn)(LPFILETIME);

...and the function we're trying to assign to a variable of that type
is declared like this:

static void init_gettimeofday(LPFILETIME lpSystemTimeAsFileTime);

Either "void" does not mean the same thing as "VOID", which would be
exceedingly evil, or the WINAPI in there matters, which is still evil,
but somewhat more understandable.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Thank you Robert. I have built latest source code (master branch) with MSVC
2013, it seem working fine i.e.

Build succeeded.
>     0 Warning(s)
>     0 Error(s)


I will try to mimic currawong environment (MSVC 2008) to reproduce the
issue. Thanks.


On Sat, Feb 21, 2015 at 11:59 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Sat, Feb 21, 2015 at 1:00 PM, Robert Haas <robertmhaas@gmail.com>
> wrote:
> > On Sat, Feb 21, 2015 at 12:20 PM, Robert Haas <robertmhaas@gmail.com>
> wrote:
> >> On Thu, Feb 12, 2015 at 9:59 PM, Michael Paquier
> >> <michael.paquier@gmail.com> wrote:
> >>> I looked at your patch and tested it with msvc and it looks good to me.
> >>
> >> Committed with cosmetic adjustments.  Hopefully one of you fine
> >> gentlemen will jump in to help if this turns out to break, because I
> >> don't do Windows.
> >
> > currawong doesn't like this:
> >
> > "c:\prog\bf\root\HEAD\pgsql.build\pgsql.sln" (default target) (1) ->
> > (postgres target) ->
> >   .\src\port\gettimeofday.c(53): error C2440: 'initializing' : cannot
> > convert from 'void (__cdecl *)(LPFILETIME)' to 'PgGetSystemTimeFn'
> >
> >
> > "c:\prog\bf\root\HEAD\pgsql.build\pgsql.sln" (default target) (1) ->
> > (misc\libpgport target) ->
> >   .\src\port\gettimeofday.c(53): error C2440: 'initializing' : cannot
> > convert from 'void (__cdecl *)(LPFILETIME)' to 'PgGetSystemTimeFn'
> >
> >     0 Warning(s)
> >     2 Error(s)
>
> I assume that the problem here must be whatever secret sauce WINAPI
> pours on the declaration.  PgSystemTimeFn is declared like this:
>
> typedef VOID (WINAPI *PgGetSystemTimeFn)(LPFILETIME);
>
> ...and the function we're trying to assign to a variable of that type
> is declared like this:
>
> static void init_gettimeofday(LPFILETIME lpSystemTimeAsFileTime);
>
> Either "void" does not mean the same thing as "VOID", which would be
> exceedingly evil, or the WINAPI in there matters, which is still evil,
> but somewhat more understandable.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Robert Haas <robertmhaas@gmail.com> writes:
> I assume that the problem here must be whatever secret sauce WINAPI
> pours on the declaration.

A bit of googling says that that expands to _stdcall which is not like
_cdecl.  I tried fixing this blindly; might not work, but it can't be
any more broken.

            regards, tom lane