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

On Sat, Dec 17, 2022 at 12:55:24AM +0100, Daniel Watzinger wrote:
> Well, this is embarassing. Sorry for the inconvenience. Some part
> of my company's network infrastruture must have mangled the attachment.
> Both mails were sent using a combination of git format-patch
> and git send-email. However, as this is my first foray into this
> email-based workflow, I won't rule out a failure on my part. Bear
> with me and let's try again.

No problem. You got the email and the patch format rights!

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?

The patch has some formatting problems, see git diff --check for
example. This does not prevent looking at the patch.

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.

- 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?

+ 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?

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?

checkSeek(FILE *fp)
{
pgoff_t tpos;
+ struct stat st;
+
+ /* Check if this is a terminal descriptor */
+ if (isatty(fileno(fp))) {
+ return false;
+ }
+
+ /* Check if this is an unseekable character special device or pipe */
+ if ((fstat(fileno(fp), &st) == 0) && (S_ISCHR(st.st_mode)
+ || S_ISFIFO(st.st_mode))) {
+ return false;
+ }
Using that without a control of WIN32 is disturbing, but that comes
down to if we'd want to tackle that within an extra layer of
fseek()/ftello() in the WIN32 port.

I am adding Juan in CC, as I am sure he'd have comments to offer on
this area of the code.
--
Michael