Re: pg_dump/pg_restore: Fix stdin/stdout handling of custom format on Win32

On Thu, Mar 2, 2023 at 8:01 AM Michael Paquier <michael@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