From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> |
Cc: | tgl(at)sss(dot)pgh(dot)pa(dot)us, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: More consistency for some file-related error message |
Date: | 2018-07-19 05:35:42 |
Message-ID: | 20180719053542.GJ3411@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | David Fetter | 2018-07-19 05:52:18 | Re: Make foo=null a warning by default. |
Previous Message | Charles Cui | 2018-07-19 05:12:44 | project updates |