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

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