Lists: | pgsql-patches |
---|
From: | Magnus Hagander <magnus(at)hagander(dot)net> |
---|---|
To: | pgsql-patches(at)postgresql(dot)org |
Subject: | Fix for win32 stat() problems |
Date: | 2008-04-09 17:18:54 |
Message-ID: | 20080409191854.67cf8bc7@mha-laptop |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Attached is a patch that attempts to fix the issues with stat() not
properly updating st_size on win32, as reported in this thread:
http://archives.postgresql.org/pgsql-hackers/2008-03/msg01181.php
It has to have a chance to affect things beyond just the
pg_relation_size() function, so this patch fixes all cases where we do
stat() and use the st_size member. It doesn't change all occurances of
stat() since I didn't want to incur the double filesystem lookups
unnecessary cases.
Any objections?
//Magnus
Attachment | Content-Type | Size |
---|---|---|
win32_stat.diff | text/x-patch | 4.9 KB |
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Magnus Hagander <magnus(at)hagander(dot)net> |
Cc: | pgsql-patches(at)postgresql(dot)org |
Subject: | Re: Fix for win32 stat() problems |
Date: | 2008-04-09 18:14:46 |
Message-ID: | 24350.1207764886@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Magnus Hagander <magnus(at)hagander(dot)net> writes:
> + #ifndef WIN32
> if (stat(xlogpath, &stat_buf) == 0)
> + #else
> + if (pgwin32_safestat(xlogpath, &stat_buf) == 0)
> + #endif
Ick. Please do this the way we normally do things when we have to
override broken Windows syscalls, that is put something like
#define stat(...) pgwin32_stat(...)
into the win32 port header file, so the calls don't have to be
nonstandard.
regards, tom lane
From: | Magnus Hagander <magnus(at)hagander(dot)net> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pgsql-patches(at)postgresql(dot)org |
Subject: | Re: Fix for win32 stat() problems |
Date: | 2008-04-09 18:20:29 |
Message-ID: | 20080409202029.7f196767@mha-laptop |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
> > + #ifndef WIN32
> > if (stat(xlogpath, &stat_buf) == 0)
> > + #else
> > + if (pgwin32_safestat(xlogpath, &stat_buf) == 0)
> > + #endif
>
> Ick. Please do this the way we normally do things when we have to
> override broken Windows syscalls, that is put something like
>
> #define stat(...) pgwin32_stat(...)
>
> into the win32 port header file, so the calls don't have to be
> nonstandard.
The reason not to do so was to avoid having to do the two filesystem
calls for *every* stat, instead just calling them both when we actually
need to use the st_size member. I take it you don't think that's a good
enough reason, but I just want to be sure you're aware that's why I did
it the way I did.
Or do you by any chance now a better way to accomplish that goal? :-)
//Magnus
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Magnus Hagander <magnus(at)hagander(dot)net> |
Cc: | pgsql-patches(at)postgresql(dot)org |
Subject: | Re: Fix for win32 stat() problems |
Date: | 2008-04-09 19:48:53 |
Message-ID: | 26499.1207770533@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Tom Lane wrote:
>> Ick.
> The reason not to do so was to avoid having to do the two filesystem
> calls for *every* stat, instead just calling them both when we actually
> need to use the st_size member.
I don't think that's worth (a) the code uglification, or (b) the chance
of a bug later due to someone not knowing they need the special version
of stat(). Are there any stat() calls that are in sufficiently
performance-critical paths that it really matters? How much slower is
the second call, anyway?
regards, tom lane
From: | Magnus Hagander <magnus(at)hagander(dot)net> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pgsql-patches(at)postgresql(dot)org |
Subject: | Re: Fix for win32 stat() problems |
Date: | 2008-04-09 19:54:39 |
Message-ID: | 20080409215439.2968aa88@mha-laptop |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
> > Tom Lane wrote:
> >> Ick.
>
> > The reason not to do so was to avoid having to do the two filesystem
> > calls for *every* stat, instead just calling them both when we
> > actually need to use the st_size member.
>
> I don't think that's worth (a) the code uglification, or (b) the
> chance of a bug later due to someone not knowing they need the
> special version of stat(). Are there any stat() calls that are in
> sufficiently performance-critical paths that it really matters? How
> much slower is the second call, anyway?
Not sure really, the VM I'm working on now is so slow I can't measure
these things properly anyway. Probably not enough to matter compared to
other things - I'll try it that way to see if I can notice any
significant difference.
Trying to prepare a patch that does it the normal way, but so far I'm
failing rather miserably. The *struct* stat is already redefined on
win32, so whenever I try #undef or so it conflicts with that :-( Since
there is no way to #undef only the parametrized version.
Though right now I have the backend linking properly even though a
bunch of files refer to pgwin32_safestat() and I've actually removed
the implementation of the function, so maybe I just need to go to bed...
//Magnus
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Magnus Hagander <magnus(at)hagander(dot)net> |
Cc: | pgsql-patches(at)postgresql(dot)org |
Subject: | Re: Fix for win32 stat() problems |
Date: | 2008-04-09 20:26:31 |
Message-ID: | 26964.1207772791@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Trying to prepare a patch that does it the normal way, but so far I'm
> failing rather miserably. The *struct* stat is already redefined on
> win32, so whenever I try #undef or so it conflicts with that :-( Since
> there is no way to #undef only the parametrized version.
I don't follow ... there's no such redefinition in our code AFAICS.
Do you mean that the system header files declare it as a macro?
regards, tom lane
From: | Magnus Hagander <magnus(at)hagander(dot)net> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pgsql-patches(at)postgresql(dot)org |
Subject: | Re: Fix for win32 stat() problems |
Date: | 2008-04-09 20:28:10 |
Message-ID: | 20080409222810.187d1b75@mha-laptop |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
> > Trying to prepare a patch that does it the normal way, but so far
> > I'm failing rather miserably. The *struct* stat is already
> > redefined on win32, so whenever I try #undef or so it conflicts
> > with that :-( Since there is no way to #undef only the parametrized
> > version.
>
> I don't follow ... there's no such redefinition in our code AFAICS.
> Do you mean that the system header files declare it as a macro?
Yes.
//Magnus
From: | Andrew Dunstan <andrew(at)dunslane(dot)net> |
---|---|
To: | Magnus Hagander <magnus(at)hagander(dot)net> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-patches(at)postgresql(dot)org |
Subject: | Re: Fix for win32 stat() problems |
Date: | 2008-04-09 23:40:49 |
Message-ID: | 47FD5401.5000509@dunslane.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Magnus Hagander wrote:
> Tom Lane wrote:
>
>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>
>>> Trying to prepare a patch that does it the normal way, but so far
>>> I'm failing rather miserably. The *struct* stat is already
>>> redefined on win32, so whenever I try #undef or so it conflicts
>>> with that :-( Since there is no way to #undef only the parametrized
>>> version.
>>>
>> I don't follow ... there's no such redefinition in our code AFAICS.
>> Do you mean that the system header files declare it as a macro?
>>
>
> Yes.
>
>
>
How about #defining safe_stat to be pg_win32_safe_stat on Windows and
simply stat elsewhere? Then use safe_stat at the places you consider
critical.
cheers
andrew
From: | Alvaro Herrera <alvherre(at)commandprompt(dot)com> |
---|---|
To: | Andrew Dunstan <andrew(at)dunslane(dot)net> |
Cc: | Magnus Hagander <magnus(at)hagander(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-patches(at)postgresql(dot)org |
Subject: | Re: Fix for win32 stat() problems |
Date: | 2008-04-10 13:08:53 |
Message-ID: | 20080410130853.GE4697@alvh.no-ip.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Andrew Dunstan wrote:
> How about #defining safe_stat to be pg_win32_safe_stat on Windows and
> simply stat elsewhere? Then use safe_stat at the places you consider
> critical.
I would couple this with a pgwin32_unsafe_stat on Windows, which changes
the size value to 0, so that if anyone gets it wrong it's immediately
obvious.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
From: | Magnus Hagander <magnus(at)hagander(dot)net> |
---|---|
To: | |
Cc: | pgsql-patches(at)postgresql(dot)org |
Subject: | Re: Fix for win32 stat() problems |
Date: | 2008-04-10 13:23:37 |
Message-ID: | 20080410152337.042157ca@mha-laptop |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Magnus Hagander wrote:
> Attached is a patch that attempts to fix the issues with stat() not
> properly updating st_size on win32, as reported in this thread:
> http://archives.postgresql.org/pgsql-hackers/2008-03/msg01181.php
>
> It has to have a chance to affect things beyond just the
> pg_relation_size() function, so this patch fixes all cases where we do
> stat() and use the st_size member. It doesn't change all occurances of
> stat() since I didn't want to incur the double filesystem lookups
> unnecessary cases.
>
> Any objections?
Updated version. Seems the problem was that the includes came in the
wrong order - figured that out just after I went to bed :) Here's an
updated patch.
A whole lot simpler patch :-)
//Magnus
Attachment | Content-Type | Size |
---|---|---|
win32_stat.diff | text/x-patch | 1.8 KB |
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Magnus Hagander <magnus(at)hagander(dot)net> |
Cc: | pgsql-patches(at)postgresql(dot)org |
Subject: | Re: Fix for win32 stat() problems |
Date: | 2008-04-10 14:41:15 |
Message-ID: | 1335.1207838475@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Magnus Hagander <magnus(at)hagander(dot)net> writes:
> A whole lot simpler patch :-)
Seems like you no longer need the !defined(_DIRMOD_C) bit here?
Also please include a comment about why <sys/stat.h> has to be
forcibly included.
A more general question: can't we get rid of most of the #ifdef WIN32
cruft in include/port.h, and put it in include/port/win32.h instead?
Seems cleaner that way, at least for things where there's just an
#ifdef WIN32 hunk and not two cases for Win and not-Win.
regards, tom lane
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Alvaro Herrera <alvherre(at)commandprompt(dot)com> |
Cc: | Andrew Dunstan <andrew(at)dunslane(dot)net>, Magnus Hagander <magnus(at)hagander(dot)net>, pgsql-patches(at)postgresql(dot)org |
Subject: | Re: Fix for win32 stat() problems |
Date: | 2008-04-10 14:43:01 |
Message-ID: | 1380.1207838581@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Andrew Dunstan wrote:
>> How about #defining safe_stat to be pg_win32_safe_stat on Windows and
>> simply stat elsewhere? Then use safe_stat at the places you consider
>> critical.
> I would couple this with a pgwin32_unsafe_stat on Windows, which changes
> the size value to 0, so that if anyone gets it wrong it's immediately
> obvious.
It's only worth having two versions if someone can show that there's
actually going to be a performance problem from the extra syscall.
I don't believe we use stat() in any place where it's really gonna
matter much ...
regards, tom lane
From: | Magnus Hagander <magnus(at)hagander(dot)net> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pgsql-patches(at)postgresql(dot)org |
Subject: | Re: Fix for win32 stat() problems |
Date: | 2008-04-10 15:16:01 |
Message-ID: | 20080410171601.7b54ca42@mha-laptop |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
> > A whole lot simpler patch :-)
>
> Seems like you no longer need the !defined(_DIRMOD_C) bit here?
Correct. That wasn't the actual error, that was me misdiagnosing the
situation.
> Also please include a comment about why <sys/stat.h> has to be
> forcibly included.
Will do.
> A more general question: can't we get rid of most of the #ifdef WIN32
> cruft in include/port.h, and put it in include/port/win32.h instead?
> Seems cleaner that way, at least for things where there's just an
> #ifdef WIN32 hunk and not two cases for Win and not-Win.
Yeah, that one has been on my TODO for a while.
We may not be able to move everything because one file is included
early in the pass of c.h and one much later, but the majority of the
stuff we have there shouldn't care about which order it goes in at.
//Magnus
From: | Magnus Hagander <magnus(at)hagander(dot)net> |
---|---|
To: | |
Cc: | pgsql-patches(at)postgresql(dot)org |
Subject: | Re: Fix for win32 stat() problems |
Date: | 2008-04-10 17:00:10 |
Message-ID: | 20080410190010.7a0fc21a@mha-laptop |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Magnus Hagander wrote:
> Magnus Hagander wrote:
> > Attached is a patch that attempts to fix the issues with stat() not
> > properly updating st_size on win32, as reported in this thread:
> > http://archives.postgresql.org/pgsql-hackers/2008-03/msg01181.php
> >
> > It has to have a chance to affect things beyond just the
> > pg_relation_size() function, so this patch fixes all cases where we
> > do stat() and use the st_size member. It doesn't change all
> > occurances of stat() since I didn't want to incur the double
> > filesystem lookups unnecessary cases.
> >
> > Any objections?
>
> Updated version. Seems the problem was that the includes came in the
> wrong order - figured that out just after I went to bed :) Here's an
> updated patch.
>
> A whole lot simpler patch :-)
Appled with comment adjustment, and backpatched to 8.2 and 8.3.
//Magnus