On Wed, Jun 26, 2019 at 4:23 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> The former patch was rather impressive. Or scary. Or both. At which
> extent have you tested it? I think that we really need to make sure
> of a couple of things which satisfy our needs:
I wanted to make a quick test on the previous patch. So let me state
what have I tested and what I have not: it builds and pass tests in
Windows and Cygwin, but I have not setup a MinGW environment.
> 1) Are we able to fix the issues with stat() calls on files larger
> than 2GB and report a correct size?
I have successfuly tested a COPY with large files.
> 2) Are we able to detect properly that files pending for deletion are
> discarded with ENOENT?
Cannot reproduce reliably, but is using the same logic as pgwin32_safestat().
> 3) Are frontends able to use the new layer?
After removing UNSAFE_STAT_OK, is this still an issue?
> It seems to me that you don't need the configure changes.
The changes in configuration are meant for gcc compilations in Windows
(Cygwin and Mingw).
> Instead of stat_pg_fixed which is confusing because it only involves
> Windows, I would rename the new file to stat.c or win32_stat.c. The
> location in src/port/ is adapted. I would also move out of
> win32_port.h the various inline declarations and keep only raw
> declarations. That could be much cleaner.
Ok.
> The code desperately needs more comments to help understand its
> logic. Don't we have in the tree an equivalent of cvt_ft2ut? What
> does cvt_attr2uxmode do? It would be nice to avoid conversion
> wrappers as much as possible, and find out system-related equivalents
> if any, and actually if necessary.
I have only found something similar in ./src/port/gettimeofday.c, but
not sure if this patch should touch that code.
> +static unsigned short
> +cvt_attr2uxmode(int attr, const _TCHAR * name)
> This looks rather bug-prone...
I wanted to keep as much of the original code as possible, but if this
is found as a viable solution, what shape should it have?
> I think that this stuff has not been tested and would break at
> compilation. If src/tools/msvc/Mkvcbuild.pm is not changed, then the
> new file won't get included in the compiled set.
The previous patch was broken, taken from the wrong local branch
(sorry about that). The attached is still a WIP but it has to do the
things above-mentioned.
Regards,
Juan José Santamaría Flecha
