SEO

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...3/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


--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/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...3/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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches