Thread: Fix for win32 stat() problems
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
Magnus Hagander <magnus@hagander.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
Tom Lane wrote: > Magnus Hagander <magnus@hagander.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
Magnus Hagander <magnus@hagander.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
Tom Lane wrote: > Magnus Hagander <magnus@hagander.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
Magnus Hagander <magnus@hagander.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
Tom Lane wrote: > Magnus Hagander <magnus@hagander.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
Magnus Hagander wrote: > Tom Lane wrote: > >> Magnus Hagander <magnus@hagander.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
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
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
Magnus Hagander <magnus@hagander.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
Alvaro Herrera <alvherre@commandprompt.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
Tom Lane wrote: > Magnus Hagander <magnus@hagander.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
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