Re: More consistency for some file-related error message

Hello.

At Thu, 19 Jul 2018 12:33:30 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20180719033330(dot)GH3411(at)paquier(dot)xyz>
> On Wed, Jul 18, 2018 at 11:24:05PM -0400, Tom Lane wrote:
> > read() is required by spec to set errno when returning a negative result.
> > I think the previous coding paid attention to errno regardless of the sign
> > of the result, which would justify pre-zeroing it ... but the new coding
> > definitely doesn't.
>
> Yes, my point is a bit different though.. Do you think that we need to
> bother about the case where errno is not 0 before calling read(), in the
> case where it returns a positive result? This would mean that errno
> would still have a previous errno set, still it returned a number of
> bytes read. For the code paths discussed here that visibly does not
> matter so you are right, we could remove them, still patterns get easily
> copy-pasted around...

Agreed to it's not necessary and a developer ought to know about
the errno behavior. However, I can sympathize with Michael.

Meawhile I found the following code in xlog.c.

| r = fread(labelfile, statbuf.st_size, 1, lfp);
| labelfile[statbuf.st_size] = '\0';
|
| /*
| * Close and remove the backup label file
| */
| if (r != 1 || ferror(lfp) || FreeFile(lfp))
| ereport(ERROR,
| (errcode_for_file_access(),
| errmsg("could not read file \"%s\": %m",
| BACKUP_LABEL_FILE)));

fread() and ferror() don't set errno so the
errcode_for_file_access() gives a bogus result. The same found in
basebackup.c and genfile.c. Also found in bootscanner.c but it
doesn't come from .l file..

CopyGetData has a variant of it.

| bytesread = fread(databuf, 1, maxread, cstate->copy_file);
| if (ferror(cstate->copy_file))
| ereport(ERROR,
| (errcode_for_file_access(),

ereport(..(errcode_for_file_access() gets bogus errno here.

Might be trivial but the following message is emitted for
AllocateFile failure and AllocateFile feilure in other places
gets a different message "could not *open* file". The differece
leads to a slightly confusing message which doesn't harm so much
like "could not read file: File name too long"..

| - errmsg("could not read pg_stat_statement file \"%s\": %m",
| + errmsg("could not read file \"%s\": %m",

And I see other variants of this like the follows.

"could not read from hash-join temporary file: %m"
"could not read two-phase state file \"%s\": %m"
"could not read from COPY file: %m"

I'm not sure it's a good thing to elimiate all specific file naem
from all of these but I don't find a criteria whether we use
generic or specific message in each case.

About the following and similars, there's two variants which has
"to" and not has it.

| - errmsg("could not write pg_stat_statement file \"%s\": %m",
| + errmsg("could not write file \"%s\": %m",

| - errmsg("could not write to control file: %m")));
| + errmsg("could not write to file \"%s\": %m",

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center