Re: BufFileRead() error signalling

On Fri, Jun 5, 2020 at 8:14 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, Jun 05, 2020 at 06:03:59PM +1200, Thomas Munro wrote:
> > I didn't change BufFileWrite() to be void, to be friendly to existing
> > callers outside the tree (if there are any), though I removed all the
> > code that checks the return code.  We can make it void later.
>
> Missing one entry in AppendStringToManifest().

Fixed.

> Anyway, why are we sure that it is fine to not complain even if
> BufFileWrite() does a partial write?  fwrite() is mentioned at the top
> of the thread, but why is that OK?

It's not OK.  If any system call fails, we'll now ereport()
immediately.  Now there can't be unhandled or unreported errors, and
it's no longer possible for the caller to confuse EOF with errors.
Hence the change in descriptions:

- * Like fread() except we assume 1-byte element size.
+ * Like fread() except we assume 1-byte element size and report I/O errors via
+ * ereport().

- * Like fwrite() except we assume 1-byte element size.
+ * Like fwrite() except we assume 1-byte element size and report errors via
+ * ereport().

Stepping back a bit, one of the problems here is that we tried to
model the functions on <stdio.h> fread(), but we didn't provide the
companion ferror() and feof() functions, and then we were a bit fuzzy
on when errno is set, even though the <stdio.h> functions don't
document that.  There were various ways forward, but the one that this
patch follows is to switch to our regular error reporting system.  The
only thing that really costs us is marginally more vague error
messages.  Perhaps that could eventually be fixed by passing in some
more context into calls like BufFileCreateTemp(), for use in error
messages and perhaps also path names.

Does this make sense?

> +       elog(ERROR, "could not seek block %ld temporary file", blknum);
>
> You mean "in temporary file" in the new message, no?

Fixed.

> +           ereport(ERROR,
> +                   (errcode_for_file_access(),
> +                    errmsg("could not write to \"%s\" : %m",
> +                           FilePathName(thisfile))));
>
> Nit: "could not write [to] file \"%s\": %m" is a more common error
> string.

Fixed.