At Wed, 23 May 2018 09:00:40 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180523000040.GA3461@paquier.xyz>
> On Tue, May 22, 2018 at 04:51:00PM +0900, Kyotaro HORIGUCHI wrote:
> > I see the same issue in snapbuild.c(4 places).
> >
> > | readBytes = read(fd, &ondisk, SnapBuildOnDiskConstantSize);
> > | pgstat_report_wait_end();
> > | if (readBytes != SnapBuildOnDiskConstantSize)
> > | {
> > | CloseTransientFile(fd);
> > | ereport(ERROR,
> > | (errcode_for_file_access(),
> > | errmsg("could not read file \"%s\", read %d of %d: %m",
> > | path, readBytes, (int) SnapBuildOnDiskConstantSize)));
> > | }
>
> Four times the same pattern, which also bloat errno when closing the
> file descriptor. I did not catch those.
>
> > and walsender.c (2 places)
> >
> > | if (nread <= 0)
> > | ereport(ERROR,
> > | (errcode_for_file_access(),
> > | errmsg("could not read file \"%s\": %m",
> > | path)));
>
> Those two ones I saw, but I was not sure if it is worth the complication
> to error on an empty file. We could do something like the attached which
> would be an improvement in readability?
The case is not of an empty file. read() reads 0 bytes without
error while lseek have told that the file has *more* data. I
don't think that can happen. How about just commenting with
something like the following?
> nread = read(fd, rbuf, sizeof(rbuf));
> /*
> * errno is E_OK in the case where nread == 0, but that can
> * scarecely happen so we don't separate the case.
> */
> if (nread <= 0)
> ereport(ERROR,
If we ereport(%m) for the nread == 0 case, we need to initialize
errno.
> > and pg_receivewal.c
> >
> > | if (read(fd, (char *) buf, sizeof(buf)) != sizeof(buf))
> > | {
> > | fprintf(stderr, _("%s: could not read compressed file \"%s\": %s\n"),
> > | progname, fullpath, strerror(errno));
>
> Okay.
>
> > pg_waldump.c
> >
> > | if (readbytes <= 0)
> > ...
> > | fatal_error("could not read from log file %s, offset %u, length %d: %s",
> > | fname, sendOff, segbytes, strerror(err));
> >
> >
> > A bit different issue, but in pg_waldump.c, search_directory can
> > check uninitialized errno when read returns a non-zero value.
>
> Yeah, the error message could be improved as well if the result is an
> empty file.
>
> Updated patch is attached. Thanks for your review.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
