pg.ddx.io pgsql-bugs@postgresql.org mailing list archive help / color / mirror / Atom feed From: Tom Lane <tgl@sss.pgh.pa.us>
To: Juan José SantamarÃa Flecha <juanjo.santamaria@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: williamedwinallen@live.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15858: could not stat file - over 4GB
Date: Wed, 19 Jun 2019 13:40:10 -0400
Message-ID: <16138.1560966010@sss.pgh.pa.us> (raw )
In-Reply-To: <CAC+AXB0j9pN4z5XxMbn6+kcE_QM+CErjfYp-vVz2rU4suU7u=g@mail.gmail.com >
References: <15858-9572469fd3b73263@postgresql.org >
<20190619012604.GC2135@paquier.xyz >
<CAC+AXB0j9pN4z5XxMbn6+kcE_QM+CErjfYp-vVz2rU4suU7u=g@mail.gmail.com >
=?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
view thread (45+ messages) latest in thread
Message-ID: <16138.1560966010@sss.pgh.pa.us>
Permalink: ../16138.1560966010@sss.pgh.pa.us/
Also on: postgresql.org/message-id/16138.1560966010@sss.pgh.pa.us
copy link · copy postgr.es
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, juanjo.santamaria@gmail.com, michael@paquier.xyz, williamedwinallen@live.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15858: could not stat file - over 4GB
In-Reply-To: <16138.1560966010@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox