Re: BUG #15858: could not stat file - over 4GB

=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
> On Wed, Jun 19, 2019 at 3:26 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Windows is known for having limitations in its former implementations
>> of stat(), and the various _stat structures they use make actually
>> that much harder from a compatibility point of view:
>> https://www.postgresql.org/message-id/1803D792815FC24D871C00D17AE95905CF5099@g01jpexmbkw24

> Going through this discussion it is not clear to me if there was a
> consensus about the shape of an acceptable patch. Would something like
> the attached be suitable?

I think there's general agreement that the correct fix involves somehow
mapping stat() to _stat64() and mapping "struct stat" to "struct __stat64"
to go along with that.  Beyond that, things get murky.

1. Can we assume that _stat64() and struct __stat64 exist on every Windows
version and build toolchain that we care about?  Windows itself is
probably OK --- googling found a (non-authoritative) statement that these
were introduced in Windows 2K.  But it's less clear whether they'll work
on builds with Cygwin, or Mingw, or Mingw-64, or how far back that support
goes.  I found one statement that Mingw declares them only "#if
__MSVCRT_VERSION__ >= 0x0601".

2. Mapping stat() to _stat64() seems easy enough: we already declare
stat(a,b) as a macro on Windows, so just change it to something else.

3. What about the struct name?  I proposed just "define stat __stat64",
but Robert thought that was too cute, and he's got a point --- in
particular, it's not clear to me how nicely it'd play to have both
function and object macros for the same name "stat".  I see you are
proposing fixing this angle by suppressing the system definition of
struct stat and then defining it ourselves with the same contents as
struct __stat64.  That might work.  Ordinarily I'd be worried about
bit-rot in a struct that has to track a system definition, but Microsoft
are so religiously anal about never breaking ABI that it might be safe
to assume we don't have to worry about that.

I don't like the specific way you're proposing suppressing the system
definition of struct stat, though.  "#define _CRT_NO_TIME_T" seems
like it's going to be a disaster, both because it likely has other
side-effects and because it probably doesn't do what you intend at all
on non-MSVC toolchains.  We have precedents for dealing with similar
issues in, eg, plperl; and what those precedents would suggest is
doing something like

#define stat microsoft_native_stat
#include <sys/stat.h>
#undef stat

after which we could do

struct stat {
       ... same contents as __stat64
};

#define stat(a,b) _stat64(a,b)

Another issue here is that pgwin32_safestat() probably needs revisited
as to its scope and purpose.  Its use of GetFileAttributesEx() can
presumably be dropped.  I don't actually believe the header comment
claiming that stat() is not guaranteed to update the st_size field;
there's no indication of that in the Microsoft documentation.  What
seems more likely is that that's a garbled version of the truth,
that you won't get a correct value of _st_size for files over 4GB.
But the test for ERROR_DELETE_PENDING might be worth keeping.  So
that would lead us to

struct stat {
       ... same contents as __stat64
};

extern int    pgwin32_safestat(const char *path, struct stat *buf);
#define stat(a,b) pgwin32_safestat(a,b)

and something like

int
pgwin32_safestat(const char *path, struct stat *buf)
{
    int            r;

    /*
     * Don't call stat(), that would just recurse back to here.
     * We really want _stat64().
     */
    r = _stat64(path, buf);

    if (r < 0)
    {
        if (GetLastError() == ERROR_DELETE_PENDING)
        {
            /*
             * File has been deleted, but is not gone from the filesystem yet.
             * This can happen when some process with FILE_SHARE_DELETE has it
             * open and it will be fully removed once that handle is closed.
             * Meanwhile, we can't open it, so indicate that the file just
             * doesn't exist.
             */
            errno = ENOENT;
        }
    }
    return r;
}

Not sure if we'd need an explicit cast to override passing struct
stat * to _stat64().  If so, a StaticAssert that sizeof(struct stat)
matches sizeof(struct __stat64) seems like a good idea.

I'd also be very strongly inclined to move pgwin32_safestat into its
own file in src/port and get rid of UNSAFE_STAT_OK.  There wouldn't
be a good reason to opt out of using it once we got to this point.

            regards, tom lane