We had better make sure that this does not break again 10260c7, and
these could not be reproduced with automated tests as they needed a
Windows terminal. Isn't this issue like the other commit, where the
automated testing cannot reproduce any of that because it requires a
terminal? If not, could it be possible to add some tests to have some
coverage? The tests of pg_dump in src/bin/pg_dump/t/ invoke the
custom format in a few scenarios already, and these are tested in the
buildfarm for a couple of years now, without failing, but perhaps we'd
need a small tweak to have a reproducible test case for automation?
I've been able to manually reproduce the problem with:
pg_dump --format=custom > custom.dump
pg_restore --file=toc.txt --list custom.dump
pg_dump --format=custom | pg_restore --file=toc.dump --use-list=toc.txt
The error I get is:
pg_restore: error: unsupported version (0.7) in file header
I'm not really sure how to integrate this in a tap test.
The internal implementation of _pgstat64() is used in quite a few
places, so we'd better update this part first, IMO, and then focus on
the pg_dump part. Anyway, it looks like you are right here: there is
nothing for FILE_TYPE_PIPE or FILE_TYPE_CHAR in this WIN32
implementation of fstat().
I am amazed to hear that both ftello64() and fseek64() actually
succeed if you use a pipe:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/fseek.html
Could it be something we should try to make more portable by ourselves
with a wrapper for these on WIN32? That would not be the first one to
accomodate our code with POSIX, and who knows what code could be broken
because of that, like external extensions that use fseek64() without
knowing it.
The error is reproducible in versions previous to win32stat.c, so that might work as bug fix.
- if (hFile == INVALID_HANDLE_VALUE || buf == NULL)
+ if (hFile == INVALID_HANDLE_VALUE || hFile == (HANDLE)-2 || buf == NULL)
What's the -2 for? Perhaps this should have a comment?
There's a note on _get_osfhandle() [1] about when -2 is returned, but a comment seems appropriate.
+ fileType = GetFileType(hFile);
+ lastError = GetLastError();
[...]
if (fileType == FILE_TYPE_UNKNOWN && lastError != NO_ERROR) {
+ _dosmaperr(lastError);
+ return -1;
}
So, the patched code assumes that all the file types classified as
FILE_TYPE_UNKNOWN when GetFileType() does not fail refer to fileno
being either stdin, stderr or stdout. Perhaps we had better
cross-check that fileno points to one of these three cases in the
switch under FILE_TYPE_UNKNOWN? Could there be other cases where we
have FILE_TYPE_UNKNOWN but GetFileType() does not fail?
I don't think we should set st_mode for FILE_TYPE_UNKNOWN.
Per the documentation of GetFileType, FILE_TYPE_REMOTE is unused:
https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfiletype
Perhaps it would be safer to fail in this case?
+1, we don't know what that might involve.
Regards,
Juan José Santamaría Flecha
