Fix for win32 stat() problems

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

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