From: Emil Iggland <emil@iggland.com>
To: pgsql-hackers@lists.postgresql.org
Cc: Juanjo Santamaria Flecha <juanjo.santamaria@gmail.com>
Subject: Re: BUG #15858: could not stat file - over 4GB
Date: Wed, 07 Oct 2020 19:13:29 +0000
Message-ID: <160209800917.1171.12856112208955258625.pgcf@coridan.postgresql.org> (raw)
In-Reply-To: <CAC+AXB2164H1JjSOPwf=PK5AhRVHSApzC5NAU-443HSu7RotSA@mail.gmail.com>
References: <15858-9572469fd3b73263@postgresql.org>
<CAC+AXB2164H1JjSOPwf=PK5AhRVHSApzC5NAU-443HSu7RotSA@mail.gmail.com>
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
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: emil@iggland.com, pgsql-hackers@lists.postgresql.org, juanjo.santamaria@gmail.com
Subject: Re: BUG #15858: could not stat file - over 4GB
In-Reply-To: <160209800917.1171.12856112208955258625.pgcf@coridan.postgresql.org>
* 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