Re: [Windows,PATCH] Use faster, higher precision timer API

Hi all

Attached is a patch to switch 9.5 over to using the
GetSystemTimeAsFileTime call instead of separate GetSystemTime and
SystemTimeToFileTime calls.

This patch the first step in improving PostgreSQL's support for Windows
high(er) resolution time.

In addition to requiring one less call into the platform libraries, this
change permits capture of timestamps at up to 100ns precision, instead
of the current 1ms limit. Unfortunately due to platform timer resolution
limitations it will in practice only report with 1ms resolution and
0.1ms precision - or sometimes even as much as 15ms resolution. (If you
want to know more, see the README for
https://github.com/2ndQuadrant/pg_sysdatetime).

On Windows 2012 and Windows 8 I'd like to use the new
GetSystemTimePreciseAsFileTime call instead. As this requires some extra
hoop-jumping to safely and efficiently use it without breaking support
for older platforms I suggest that we start with just switching over to
GetSystemTimeAsFileTime, which has been supported since Windows 2000.
Then more precise time capture can be added in a later patch.

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

On 09/17/2014 08:27 AM, Craig Ringer wrote:
> Hi all
>
> Attached is a patch to switch 9.5 over to using the
> GetSystemTimeAsFileTime call instead of separate GetSystemTime and
> SystemTimeToFileTime calls.
>
> This patch the first step in improving PostgreSQL's support for Windows
> high(er) resolution time.
>
> In addition to requiring one less call into the platform libraries, this
> change permits capture of timestamps at up to 100ns precision, instead
> of the current 1ms limit. Unfortunately due to platform timer resolution
> limitations it will in practice only report with 1ms resolution and
> 0.1ms precision - or sometimes even as much as 15ms resolution. (If you
> want to know more, see the README for
> https://github.com/2ndQuadrant/pg_sysdatetime).
>
> On Windows 2012 and Windows 8 I'd like to use the new
> GetSystemTimePreciseAsFileTime call instead. As this requires some extra
> hoop-jumping to safely and efficiently use it without breaking support
> for older platforms I suggest that we start with just switching over to
> GetSystemTimeAsFileTime, which has been supported since Windows 2000.
> Then more precise time capture can be added in a later patch.
>
>

That will presumably breaK XP. I know XP has been declared at EOL, but
there are still a heck of a lot of such systems out there, especially in
places like ATMs, but I saw it in use recently at a US surgical facility
(which is slightly scary, although this wasn't for life-sustaining
functionality). My XP system is still actually getting some security
updates sent from Microsoft.

I'm fine with doing this - frogmouth and currawong would retire on the
buildfarm.

Just wanted to be up front about it.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 09/17/2014 08:27 AM, Craig Ringer wrote:
>> Attached is a patch to switch 9.5 over to using the
>> GetSystemTimeAsFileTime call instead of separate GetSystemTime and
>> SystemTimeToFileTime calls.

> That will presumably breaK XP. I know XP has been declared at EOL, but
> there are still a heck of a lot of such systems out there,

Yeah. Do we really think more precise timestamps are worth dropping
XP support? On the Unix side, I know exactly what would happen to a
patch proposing that we replace gettimeofday() with clock_gettime()
with no thought for backwards compatibility. Why would we expect
less on the Windows side?

Quite aside from XP ... AFAICS from the patch description, this patch
in itself moves us to a place that's a net negative in terms of
functionality. Maybe it's a stepping stone to something better,
but I think we should just go directly to the something better.
I don't care for committing regressions on the promise that they'll
get fixed later.

Or in short: let's do the work needed to adapt our code to what's
available on the particular Windows version *first*. Once we've
got that configuration support done, it shouldn't be much extra
work to continue XP support here.

regards, tom lane

On 2014-09-17 11:19:36 -0400, Andrew Dunstan wrote:
>
> On 09/17/2014 08:27 AM, Craig Ringer wrote:
> >Hi all
> >
> >Attached is a patch to switch 9.5 over to using the
> >GetSystemTimeAsFileTime call instead of separate GetSystemTime and
> >SystemTimeToFileTime calls.
> >
> >This patch the first step in improving PostgreSQL's support for Windows
> >high(er) resolution time.
> >
> >In addition to requiring one less call into the platform libraries, this
> >change permits capture of timestamps at up to 100ns precision, instead
> >of the current 1ms limit. Unfortunately due to platform timer resolution
> >limitations it will in practice only report with 1ms resolution and
> >0.1ms precision - or sometimes even as much as 15ms resolution. (If you
> >want to know more, see the README for
> >https://github.com/2ndQuadrant/pg_sysdatetime).
> >
> >On Windows 2012 and Windows 8 I'd like to use the new
> >GetSystemTimePreciseAsFileTime call instead. As this requires some extra
> >hoop-jumping to safely and efficiently use it without breaking support
> >for older platforms I suggest that we start with just switching over to
> >GetSystemTimeAsFileTime, which has been supported since Windows 2000.
> >Then more precise time capture can be added in a later patch.
> >
> >
>
>
> That will presumably breaK XP.

The proposed patch? I don't really see why? GetSystemTimeAsFileTime() is
documented to be available since win2k?

Or do you mean GetSystemTimePreciseAsFileTime()? That'd surely - as
indicated by Craig - would have to be optional since it's not available
anywhere but 2012 and windows 8?

> I know XP has been declared at EOL, but there
> are still a heck of a lot of such systems out there, especially in places
> like ATMs, but I saw it in use recently at a US surgical facility (which is
> slightly scary, although this wasn't for life-sustaining functionality). My
> XP system is still actually getting some security updates sent from
> Microsoft.

I unfortunately have to agree, dropping XP is probably at least a year
or two out.

Greetings,

Andres Freund

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

On 09/17/2014 12:51 PM, Andres Freund wrote:
> On 2014-09-17 11:19:36 -0400, Andrew Dunstan wrote:
>> On 09/17/2014 08:27 AM, Craig Ringer wrote:
>>> Hi all
>>>
>>> Attached is a patch to switch 9.5 over to using the
>>> GetSystemTimeAsFileTime call instead of separate GetSystemTime and
>>> SystemTimeToFileTime calls.
>>>
>>> This patch the first step in improving PostgreSQL's support for Windows
>>> high(er) resolution time.
>>>
>>> In addition to requiring one less call into the platform libraries, this
>>> change permits capture of timestamps at up to 100ns precision, instead
>>> of the current 1ms limit. Unfortunately due to platform timer resolution
>>> limitations it will in practice only report with 1ms resolution and
>>> 0.1ms precision - or sometimes even as much as 15ms resolution. (If you
>>> want to know more, see the README for
>>> https://github.com/2ndQuadrant/pg_sysdatetime).
>>>
>>> On Windows 2012 and Windows 8 I'd like to use the new
>>> GetSystemTimePreciseAsFileTime call instead. As this requires some extra
>>> hoop-jumping to safely and efficiently use it without breaking support
>>> for older platforms I suggest that we start with just switching over to
>>> GetSystemTimeAsFileTime, which has been supported since Windows 2000.
>>> Then more precise time capture can be added in a later patch.
>>>
>>>
>>
>> That will presumably breaK XP.
> The proposed patch? I don't really see why? GetSystemTimeAsFileTime() is
> documented to be available since win2k?

Oh, hmm, yes, you're right. For some reason I was thinking W2K was later
than XP. I get more random memory errors as I get older ...

cheers

andrew

On 2014-09-17 09:38:59 -0700, Tom Lane wrote:
> On the Unix side, I know exactly what would happen to a
> patch proposing that we replace gettimeofday() with clock_gettime()
> with no thought for backwards compatibility.

Btw, do you plan to pursue clock_gettime()? It'd be really neat to have
it...

>
> Quite aside from XP ... AFAICS from the patch description, this patch
> in itself moves us to a place that's a net negative in terms of
> functionality. Maybe it's a stepping stone to something better, but I
> think we should just go directly to the something better. I don't
> care for committing regressions on the promise that they'll get fixed
> later.

I don't think there's any regressions in that patch? Rather the
contrary. I understand the comment about the timer tick to be just as
applicable to the current code as the new version. Just that the old
code can't possibly have a precision lower than 1ms, but the new one
can.

Greetings,

Andres Freund

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

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2014-09-17 09:38:59 -0700, Tom Lane wrote:
>> On the Unix side, I know exactly what would happen to a
>> patch proposing that we replace gettimeofday() with clock_gettime()
>> with no thought for backwards compatibility.

> Btw, do you plan to pursue clock_gettime()? It'd be really neat to have
> it...

It's on my TODO list, but not terribly close to the top. If you're
excited about that, feel free to take it up.

regards, tom lane

On 09/17/2014 11:19 PM, Andrew Dunstan wrote:
>
> On 09/17/2014 08:27 AM, Craig Ringer wrote:
>> Hi all
>> On Windows 2012 and Windows 8 I'd like to use the new
>> GetSystemTimePreciseAsFileTime call instead. As this requires some extra
>> hoop-jumping to safely and efficiently use it without breaking support
>> for older platforms I suggest that we start with just switching over to
>> GetSystemTimeAsFileTime, which has been supported since Windows 2000.
>> Then more precise time capture can be added in a later patch.

> That will presumably breaK XP.

Yes, and Windows 7. But this patch doesn't to that, it just makes
adjustments that make it easier.

The next step is to use LoadLibrary and GetProcAddress to resolve
GetSystemTimePreciseAsFileTime *if it is available*, during backend
start. Then use it if possible, and fall back to GetSystemTimeAsFileTime
if it isn't.

This patch does not introduce any BC changes. At all. I should've
omitted all mention of the next step I want to take, but I thought it
was a useful explanation of why this change makes a bigger improvement
easier.

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

On 09/18/2014 12:58 AM, Andrew Dunstan wrote:
>
> Oh, hmm, yes, you're right. For some reason I was thinking W2K was later
> than XP. I get more random memory errors as I get older ...

It's because people say Win2k3 / Win2k8 / Win2k8r2 / Win2k12 a lot as
shorthand for Windows Server 2003 (XP-based), Windows Server 2008 (Vista
based), Windows Server 2008 R2 (Windows 7 based) and Windows Server 2012
(Windows 8 based) respectively.

Win2k is just Windows 2000, the release before Windows XP, released in
December 1999. Needless to say, if it's compatible even as far back as
Win2k it's not going to worry anybody.

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

On 09/17/2014 08:27 PM, Craig Ringer wrote:
> Hi all
>
> Attached is a patch to switch 9.5 over to using the
> GetSystemTimeAsFileTime call instead of separate GetSystemTime and
> SystemTimeToFileTime calls.

Following on from my prior patch that switches to using
GetSystemTimeAsFileTime, I now attach a two-patch series that also adds
support for GetFileTimePreciseAsFileTime where it is available.

Details in the patch commit messages.

These should apply fine with "git am", or at worst "git am --3way".
Otherwise you can just grab them from the working tree:

https://github.com/ringerc/postgres/tree/win32_use_filetime

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

On Fri, Oct 10, 2014 at 10:08 PM, Craig Ringer <craig(at)2ndquadrant(dot)com>
wrote:

> On 09/17/2014 08:27 PM, Craig Ringer wrote:
> > Hi all
> >
> > Attached is a patch to switch 9.5 over to using the
> > GetSystemTimeAsFileTime call instead of separate GetSystemTime and
> > SystemTimeToFileTime calls.
>
> Following on from my prior patch that switches to using
> GetSystemTimeAsFileTime, I now attach a two-patch series that also adds
> support for GetFileTimePreciseAsFileTime where it is available.
>
>
Hi Craig,

I was just having a quick look at this with the view of testing it on a
windows 8 machine.

Here's a couple of things I've noticed:

+ pg_get_system_time = (PgGetSystemTimeFn) GetProcAddress(
+ GetModuleHandle(TEXT("kernel32.dll")),
+ "GetSystemRimePreciseAsFileTime");

"Rime", not doubt is meant to be "Time".

Same here:

+ elog(DEBUG1, "GetProcAddress(\"GetSystemRimePreciseAsFileTime\") on
kernel32.dll failed with error code %d not expected
ERROR_PROC_NOT_FOUND(127)", errcode);

But I don't think you'll be able to elog quite that early. I tested by
getting rid of the if (errcode != ERROR_PROC_NOT_FOUND) test and I get:

D:\Postgres\install\bin>postgres -D ..\data
error occurred at src\port\gettimeofday.c:87 before error message
processing is available

Perhaps we needn't bother with this debug message? Either that you'll
probably need to cache the error code and do something when the logger is
initialised.

Also, just for testing the resolution of the 2 functions, I added some code
into PostgreSQL's gettimeofday()

do{
(*pg_get_system_time)(&ft2);
}while ((file_time.dwLowDateTime - ft2.dwLowDateTime) == 0);

#ifndef FRONTEND
elog(NOTICE, "%d", ft2.dwLowDateTime - file_time.dwLowDateTime);
if (pg_get_system_time == &GetSystemTimeAsFileTime)
elog(NOTICE, "GetSystemTimeAsFileTime");
else
elog(NOTICE, "GetSystemTimePreciseAsFileTime");
#endif

return 0;

I see:
test=# select current_timestamp;
NOTICE: 4
NOTICE: GetSystemTimePreciseAsFileTime

Which indicates that this is quite a precise timer.

Whereas if I add a quick hack into init_win32_gettimeofday() so that it
always chooses GetSystemTimeAsFileTime() I see:

test=# select current_timestamp;
NOTICE: 9953
NOTICE: GetSystemTimeAsFileTime

Also, I've attached gettimeofday.c, which is a small test programme which
just makes 10 million calls to each function, in order to find out which
one of the 3 method performs best. Here I just wanted to ensure that we'd
not get any performance regression in gettimeofday() calls.

Here's the output from running this on this windows 8.1 laptop.

D:\>cl /Ox gettimeofday.c
Microsoft (R) C/C++ Optimizing Compiler Version 17.00.61030 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

gettimeofday.c
Microsoft (R) Incremental Linker Version 11.00.61030.0
Copyright (C) Microsoft Corporation. All rights reserved.

/out:gettimeofday.exe
gettimeofday.obj

D:\>gettimeofday.exe
GetSystemTimePreciseAsFileTime() in 0.157480 seconds
GetSystemTimeAsFileTime() in 0.060075 seconds
Current Method in 0.742677 seconds

Regards

David Rowley

On 10/22/2014 04:12 PM, David Rowley wrote:

> I was just having a quick look at this with the view of testing it on a
> windows 8 machine.

Thankyou. I really appreciate your taking the time to do this, as one of
the barriers to getting Windows-specific patches accepted is that
usually people just don't want to review Windows patches.

I see you've already linked it on the commitfest too, so thanks again.

I'll follow up with a fixed patch and my test code shortly. I'm at PGEU
in Madrid so things are a bit chaotic, but I can make some time.

> +pg_get_system_time = (PgGetSystemTimeFn) GetProcAddress(
> +GetModuleHandle(TEXT("kernel32.dll")),
> +"GetSystemRimePreciseAsFileTime");
>
>
> "Rime", not doubt is meant to be "Time".

Hm.

I must've somehow managed to attach and post the earlier version of the
patch before I fixed a few issues and tested it, because I've compiled
and tested this feature on Win2k12.

Apparently just not the version I published.

Thanks for catching that. I'll fix it up.

> +elog(DEBUG1, "GetProcAddress(\"GetSystemRimePreciseAsFileTime\") on
> kernel32.dll failed with error code %d not expected
> ERROR_PROC_NOT_FOUND(127)", errcode);
>
> But I don't think you'll be able to elog quite that early. I tested by
> getting rid of the if (errcode != ERROR_PROC_NOT_FOUND) test and I get:
>
> D:\Postgres\install\bin>postgres -D ..\data
> error occurred at src\port\gettimeofday.c:87 before error message
> processing is available

Thankyou. I didn't consider that logging wouldn't be available there.

This case shouldn't really happen.

> Perhaps we needn't bother with this debug message? Either that you'll
> probably need to cache the error code and do something when the logger
> is initialised.

In a "shouldn't happen" case like this I think it'll be OK to just print
to stderr.

> I see:
> test=# select current_timestamp;
> NOTICE: 4
> NOTICE: GetSystemTimePreciseAsFileTime
>
> Which indicates that this is quite a precise timer.

Great.

Because I was testing on AWS I wasn't getting results that fine, but the
kind of granularity you're seeing is consistent with what I get on my
Linux laptop.

> Whereas if I add a quick hack into init_win32_gettimeofday() so that it
> always chooses GetSystemTimeAsFileTime() I see:
>
> test=# select current_timestamp;
> NOTICE: 9953
> NOTICE: GetSystemTimeAsFileTime

I'll publish the test code I was using too. I was doing it from SQL
level with no code changes other than the ones required for timestamp
precision.

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

Here's an updated patch addressing David's points.

I haven't had a chance to test it yet, on win2k8 or win2k12 due to
pgconf.eu .

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

On Thu, Oct 23, 2014 at 1:27 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:

> Here's an updated patch addressing David's points.
>
> I haven't had a chance to test it yet, on win2k8 or win2k12 due to
> pgconf.eu .
>
>
Hi Craig, thanks for the fast turnaround.

I've just had a look over the patch again:

+ DWORD errcode = GetLastError();
+ Assert(errcode == ERROR_PROC_NOT_FOUND);

I'm not a big fan of this. It seems quite strange to be using Assert in
this way. I'd rather see any error just silently fall back
on GetSystemTimeAsFileTime() instead of this. I had originally assumed that
you stuck the debug log in there so that people would have some sort of way
of finding out if their system is using GetSystemTimePreciseAsFileTime() or
GetSystemTimeAsFileTime(), the assert's not really doing this. I'd vote
for, either removing this assert or sticking some elog DEBUG1 sometime
after the logger has started. Perhaps just a test like:

if (pg_get_system_time == &GetSystemTimeAsFileTime)
elog(DEBUG1, "gettimeofday is using GetSystemTimeAsFileTime()");
else
elog(DEBUG1, "gettimeofday is using GetSystemTimePreciseAsFileTime()");

But perhaps it's not worth the trouble.

Also if you decide to get rid of the elog, probably should also remove the
include of elog.h that you've added. Or if you disagree with my comment on
the Assert() you'll need to include the proper header for that. The
compiler is currently giving a warning about that.

Regards

David Rowley

On 10/23/2014 11:41 AM, David Rowley wrote:
> I'm not a big fan of this. It seems quite strange to be using Assert in
> this way. I'd rather see any error just silently fall back
> on GetSystemTimeAsFileTime() instead of this.

That's fair. I'd like some visibility into it, but I don't think it's vital.

> I had originally assumed
> that you stuck the debug log in there so that people would have some
> sort of way of finding out if their system is
> using GetSystemTimePreciseAsFileTime() or GetSystemTimeAsFileTime()

No, that was never the goal. The previous code using elog only logged if
the system couldn't load GetSystemTimePreciseAsFileTime() because of an
error other than the expected one when the symbol can't be found.

In other words, if you're on win2k8 nothing happens, it just silently
uses GetSystemTimeAsFileTime(). We expect failure to load the proc
address, that's ok, we just assume it's an older windows. If the load
fails for some _other_ reason though, that's a weird issue that's worth
complaining about, but we don't know anything more than "something isn't
right here".

> if (pg_get_system_time == &GetSystemTimeAsFileTime)
> elog(DEBUG1, "gettimeofday is using GetSystemTimeAsFileTime()");
> else
> elog(DEBUG1, "gettimeofday is using GetSystemTimePreciseAsFileTime()");
>
> But perhaps it's not worth the trouble.

That's probably not really worth it; it's completey different to what
the prior code was doing anyway.

> Also if you decide to get rid of the elog, probably should also remove
> the include of elog.h that you've added.

Rather.

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

On Thu, Oct 23, 2014 at 5:41 AM, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> On Thu, Oct 23, 2014 at 1:27 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> Here's an updated patch addressing David's points.
>> I haven't had a chance to test it yet, on win2k8 or win2k12 due to
>> pgconf.eu .
>>
> Hi Craig, thanks for the fast turnaround.
>
> I've just had a look over the patch again:
>
> + DWORD errcode = GetLastError();
> + Assert(errcode == ERROR_PROC_NOT_FOUND);
>
> I'm not a big fan of this. It seems quite strange to be using Assert in this
> way.

Agreed - I think if you want an error check here it should use elog()
or ereport(), not Assert().

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

On 10/23/2014 09:21 PM, Robert Haas wrote:
> Agreed - I think if you want an error check here it should use elog()
> or ereport(), not Assert().

That's what I originally did, but it's too early for elog.

I'm reluctant to just fprintf(...) to stderr, as there's no way for the
user to suppress that, and it'll be emitted for each backend start.
Though on the other hand it really is a "shouldn't happen" case.

So the options seem to be ignoring the error silently or printing to stderr.

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

On Fri, Oct 24, 2014 at 1:42 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 10/23/2014 09:21 PM, Robert Haas wrote:
>> Agreed - I think if you want an error check here it should use elog()
>> or ereport(), not Assert().
>
> That's what I originally did, but it's too early for elog.
>
> I'm reluctant to just fprintf(...) to stderr, as there's no way for the
> user to suppress that, and it'll be emitted for each backend start.
> Though on the other hand it really is a "shouldn't happen" case.
>
> So the options seem to be ignoring the error silently or printing to stderr.

Either of those is OK with me. I think it's a bad idea to use
Assert() to check the results of system calls, because an assertion
failure is supposed to indicate a bug in our code, not Microsoft's
code. But printing to stderr is an acceptable way of indicating an
error that happens very early, and ignoring doesn't look unreasonable
in this context either. Yet another option is to do nothing about the
error at the time that it's reported but store the error code
somewhere and use it to generate an error message once the system is
initialized. I'm tentatively inclined to believe that's more
machinery than this particular case justifies, but will happily defer
to any emerging consensus.

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

Hi all

I've attached a revised patchset for GetSystemTimeAsFileTime and
GetSystemTimePreciseAsFileTime to address David Rowley's review notes.

Thanks for the review, and for poking me to follow up.

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

Il 01/12/14 14:16, Craig Ringer ha scritto:
>
> +#ifndef FRONTEND
> +#include <utils/elog.h>
> +#endif
> +

I think this is a leftover, as you don't use elog afterwards.

Regards,
Marco

--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco(dot)nenciarini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

On 12/01/2014 09:51 PM, Marco Nenciarini wrote:
> I think this is a leftover, as you don't use elog afterwards.

Good catch, fixed.

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

On 2 December 2014 at 15:36, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:

> On 12/01/2014 09:51 PM, Marco Nenciarini wrote:
> > I think this is a leftover, as you don't use elog afterwards.
>
> Good catch, fixed.
>
>
I've looked over this again and tested it on a windows 8.1 machine. I
cannot find any problems

The only comments about the code I have would maybe be to use some
constants like:

#define FILETIME_PER_SEC 10000000L
#define FILETIME_PER_USEC 10

I had to read the Microsoft documentation to see that "A file time is a
64-bit value that represents the number of 100-nanosecond intervals that
have elapsed since 12:00 A.M. January 1, 1601 Coordinated Universal Time
(UTC)."

http://msdn.microsoft.com/en-us/library/windows/desktop/dn553408%28v=vs.85%29.aspx

The attached patch gets rid of those magic numbers, and hopefully makes it
a bit easier to see what's going on.

I agree with the lack of real need to log any sort of errors
if init_win32_gettimeofday() gets any unexpected errors while trying to
lookup GetSystemTimePreciseAsFileTime.

I'm marking this as ready for committer. It seems worth going in just for
the performance improvement alone, never mind the increased clock accuracy.

I'll leave it up to the committer to decide if it's better with or without
the attached patch.

Regards

David Rowley

On 12/05/2014 08:03 PM, David Rowley wrote:
> On 2 December 2014 at 15:36, Craig Ringer <craig(at)2ndquadrant(dot)com
> <mailto:craig(at)2ndquadrant(dot)com>> wrote:
>
> On 12/01/2014 09:51 PM, Marco Nenciarini wrote:
> > I think this is a leftover, as you don't use elog afterwards.
>
> Good catch, fixed.
>
> I've looked over this again and tested it on a windows 8.1 machine. I
> cannot find any problems
>
> The only comments about the code I have would maybe be to use some
> constants like:
>
> #define FILETIME_PER_SEC10000000L
> #define FILETIME_PER_USEC10

[snip]

> I'll leave it up to the committer to decide if it's better with or
> without the attached patch.

I think it's more readable, and that's pretty much always a good thing.

Patches with your changes attached.

I used FILETIME_UNITS_PER_SEC though.

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