On Thu, Jul 19, 2018 at 02:05:51PM +0900, Kyotaro HORIGUCHI wrote:
> Agreed to it's not necessary and a developer ought to know about
> the errno behavior. However, I can sympathize with Michael.
I am fine to remove them if folks here push for that.
> 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.
Yeah, I saw those but did not really bother much about them yet, errno
should be set to the return result of ferror(). If you have a patch,
please feel free to send one.
> And I see other variants of this like the follows.
>
> "could not read from hash-join temporary file: %m"
> "could not read from COPY file: %m"
This is intentional. You may not be able to guess the context based on
the file name.
> "could not read two-phase state file \"%s\": %m"
You need to refresh your tree, that does not show up on HEAD anymore.
See 811b6e3.
> 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.
I would say that we can remove any context-specific message if one can
guess easily based on the file name what the context is. For two phase
files, that's for example easy as pg_twophase/ is involved.
> 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",
It seems to me that it is important to make the distinction between a
file which gets fully written and a file which exists already and gets
written more. That's this difference I understand from these two
error messages, so that's my intention to not change them.
--
Michael
