Thread: [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
Attachment
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@dunslane.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@2ndquadrant.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
Attachment
On Fri, Oct 10, 2014 at 10:08 PM, Craig Ringer <craig@2ndquadrant.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
Attachment
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
Attachment
On Thu, Oct 23, 2014 at 1:27 AM, Craig Ringer <craig@2ndquadrant.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@gmail.com> wrote: > On Thu, Oct 23, 2014 at 1:27 AM, Craig Ringer <craig@2ndquadrant.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@2ndquadrant.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
Attachment
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.nenciarini@2ndQuadrant.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
Attachment
On 2 December 2014 at 15:36, Craig Ringer <craig@2ndquadrant.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)."
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
Attachment
On 12/05/2014 08:03 PM, David Rowley wrote: > On 2 December 2014 at 15:36, Craig Ringer <craig@2ndquadrant.com > <mailto:craig@2ndquadrant.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