Re: BufFileRead() error signalling

On Fri, Jan 24, 2020 at 11:12 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Wed, Dec 11, 2019 at 2:07 AM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
> > You are checking file->dirty twice, first before calling the function and within the function too. Same for the
Assert.For example.
 
>
> True.  Thanks for the review.  Before I post a new version, any
> opinions on whether to back-patch, and whether to do 0003 in master
> only, or at all?

Rather than answering your actual question, I'd like to complain about this:

  if (BufFileRead(file, ptr, BLCKSZ) != BLCKSZ)
- elog(ERROR, "could not read temporary file: %m");
+ elog(ERROR, "could not read temporary file");

I recognize that your commit message explains this change by saying
that this code will now never be reached except as a result of a short
read, but I don't think that will necessarily be clear to future
readers of those code, or people who get the error message. It seems
like if we're going to do do this, the error messages ought to be
changed to complain about a short read rather than an inability to
read for unspecified reasons. However, I wonder why we don't make
BufFileRead() throw all of the errors including complaining about
short reads. I would like to confess my undying (and probably
unrequited) love for the following code from md.c:

                                         errmsg("could not read block
%u in file \"%s\": read only %d of %d bytes",

Now that is an error message! I am not confused! I don't know why that
happened, but I sure know what happened!

I think we should aim for that kind of style everywhere, so that
complaints about reading and writing files are typically reported as
either of these:

could not read file "%s": %m
could not read file "%s": read only %d of %d bytes

There is an existing precedent in twophase.c which works almost this way:

        if (r != stat.st_size)
        {
                if (r < 0)
                        ereport(ERROR,
                                        (errcode_for_file_access(),
                                         errmsg("could not read file
\"%s\": %m", path)));
                else
                        ereport(ERROR,
                                        (errmsg("could not read file
\"%s\": read %d of %zu",
                                                        path, r,
(Size) stat.st_size)));
        }

I'd advocate for a couple more words in the latter message ("only" and
"bytes") but it's still excellent.

OK, now that I've waxed eloquent on that topic, let me have a go at
your actual questions. Regarding back-patching, I don't mind
back-patching error handling patches like this, but I don't feel it's
necessary if we have no evidence that data is actually getting
corrupted as a result of the problem and the chances of it actually
happening seems remote. It's worth keeping in mind that changes to
message strings will tend to degrade translatability unless the new
strings are copies of existing strings. Regarding 0003, it seems good
to me to make BufFileRead() and BufFileWrite() consistent in terms of
error-handling behavior, so +1 for the concept (but I haven't reviewed
the code).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company