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

[Craig Ringer]

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.

[Andrew Dunstan]

Post by Craig Ringer
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

[Tom Lane]

Post by Andrew Dunstan

Post by Craig Ringer
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

[Andres Freund]

Post by Tom Lane
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...

Post by Tom Lane
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

[Tom Lane]

Post by Andres Freund

Post by Tom Lane
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

[Andres Freund]

Post by Andrew Dunstan

Post by Craig Ringer
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?

Post by Andrew Dunstan
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

[Andrew Dunstan]

Post by Andres Freund

Post by Andrew Dunstan

Post by Craig Ringer
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

[Craig Ringer]

Post by Andrew Dunstan
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]

Post by Andrew Dunstan

Post by Craig Ringer
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]

Post by Craig Ringer
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

[David Rowley]

This post might be inappropriate. Click to display it.

[Craig Ringer]

Post by David Rowley
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.

Post by David Rowley
+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.

Post by David Rowley
+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
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.

Post by David Rowley
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.

Post by David Rowley
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.

Post by David Rowley
Whereas if I add a quick hack into init_win32_gettimeofday() so that it
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]

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 .

[David Rowley]

Post by Craig Ringer
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

[Craig Ringer]

Post by David Rowley
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.

Post by David Rowley
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".

Post by David Rowley
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.

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

Rather.

[Robert Haas]

Post by David Rowley

Post by Craig Ringer
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.
+ 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