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

[Emil Iggland]

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:not tested

I ran into this problem when using psql.exe and copy command. 

I have checked out 11.6-release tarball and applied the patch. 
The patch does not apply cleanly, but can be easily modified to apply. See Note 
1.
After applying the patch I built using "build psql" and ran the new psql.exe 
binary.

In order to test I have done the following: 
Against a PostgreSQL 11 server run two commands: 
"COPY public.table FROM 'C:/file'" and "\copy public.table FROM 'C:/file'"
The first one runs in the context of the server, and does not work. It aborts 
with an error saying "cannot stat file", as expected. 
The seconds on runs in the context of the new binary and does work. It copies 
data as expected. 



Note 1: 
src/tools/msvc/Mkvcbuild.pm should be 

- sprompt.c strerror.c tar.c thread.c getopt.c getopt_long.c dirent.c
- win32env.c win32error.c win32security.c win32setlocale.c);
+ sprompt.c tar.c thread.c getopt.c getopt_long.c dirent.c
+ win32env.c win32error.c win32security.c win32setlocale.c 
win32_stat.c);

The new status of this patch is: Waiting on Author

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

[Emil Iggland]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:not tested

I tested the patch at hand, and it performs as expected. Files larger than 4GB 
can be imported.

Steps: 
0) create a csv-file that is sufficiently big (>4GB), and one that is small. 
Use these files to test.
1a) Attempt to import the small file using devel-version.
1b) EXPECTED: success, ACTUAL: success
2a) Attempt to import the big file using devel-version.
2b) EXPECTED: failure, ACTUAL: failure
3) Apply patch and build new version
4a) Attempt to import the small file using patched-version.
4b) EXPECTED: success, ACTUAL: success
4a) Attempt to import the big file using patched-version.
4b) EXPECTED: success, ACTUAL: success

The code looks sensible, it is easy to read and follow. The code uses 
appropriate win32 functions to perform the task. 

Code calculates file size using the following method: buf->st_size = ((__int64) 
fiData.nFileSizeHigh) << 32 | (__int64)(fiData.nFileSizeLow);
The hard coded constant 32 is fine, nFileSizeHigh is defined as a DWORD in the 
Win32 API, which is a 32 bit unsigned integer. There is no need to a dynamic 
calculation.

There are minor "nit-picks" that I would change if it were my code, but do not 
change the functionality of the code. 

1) 
if (GetFileAttributes(name) == INVALID_FILE_ATTRIBUTES)
{
  errno = ENOENT;
  return -1;
}

Here I would call _dosmaperr(GetLastError()) instead, just to take account of 
the possibility that some other error occurred.  Following this change there 
are slight inconsistency in the order of "CloseHandle(hFile), errno = ENOENT; 
return -1" and "_dosmaperr(GetLastError()); CloseHandle(hFile); return -1". I 
would prefer consistent ordering, but that is not important.

The new status of this patch is: Ready for Committer