From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Joe Nelson <joe(at)begriffs(dot)com>, Alvaro Herrera from 2ndQuadrant <alvherre(at)alvh(dot)no-ip(dot)org>, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Surafel Temesgen <surafel3000(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Change atoi to strtol in same place |
Date: | 2019-09-11 03:53:56 |
Message-ID: | 20190911035356.GE1953@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Sep 10, 2019 at 08:03:32AM -0400, Robert Haas wrote:
> -1. I think it's very useful to have routines for this sort of thing
> that return an error message rather than emitting an error report
> directly. That gives the caller a lot more control.
Please let me counter-argue here. There are a couple of reasons to
not do as the patch proposes:
1) Consistency with the error messages makes less work for translators,
who already have a lot to deal with. The patch is awkward in this
sense, to give some examples:
+ if (s != PG_STRTOINT_OK)
{
- pg_log_error("invalid status interval \"%s\"", optarg);
+ pg_log_error("invalid status interval: %s", parse_error);
}
[...]
- pg_log_error("invalid compression level \"%s\"", optarg);
+ pg_log_error("invalid compression level: %s", parse_error);
2) A centralized error message can provide the same level of details.
Here are suggestions for each error status:
pg_log_error("could not parse value for option %s", optname);
pg_log_error("invalid value for option %s", optname);
optname should be defined by the caller with strings like
"-t/--timeout" or such. Then, if ranges are specified and the error
is on a range, I think that we should just add a second error message
to provide a hint to the user, if wanted by the caller of
pg_strtoint64_range() so an extra argument could do handle that:
pg_log_error("value must be in range %d..%d")
3) I think that we should not expose directly the status values of
pg_strtoint_status in pg_strtoint64_range(), that's less for module
authors to worry about, and that would be the same approach as we are
using for the wrappers of pg_strto[u]intXX() in the patch of the other
thread (see pg_strto[u]intXX_check for example in [1]).
[1]: https://www.postgresql.org/message-id/20190910030525.GA22934@paquier.xyz
--
Michael
From | Date | Subject | |
---|---|---|---|
Next Message | Richard Guo | 2019-09-11 03:56:33 | Re: A problem about partitionwise join |
Previous Message | Michael Paquier | 2019-09-11 02:57:29 | Re: [bug fix] Produce a crash dump before main() on Windows |