Re: More consistency for some file-related error message - Mailing list pgsql-hackers

Hello.

At Thu, 19 Jul 2018 12:33:30 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180719033330.GH3411@paquier.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