On Thu, Mar 2, 2023 at 8:01 AM Michael Paquier <mich...@paquier.xyz> wrote:
> > 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. [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle?view=msvc-170 Regards, Juan José Santamaría Flecha
