Re: Change atoi to strtol in same place

On Tue, Sep 10, 2019 at 11:54 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> 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.

OK, but on the other hand, Joe's example of a custom message "invalid
compression level: 10 is outside range 0..9" is a world better than
"invalid compression level: %s". We might even be able to do better
"argument to -Z must be a compression level between 0 and 9". In
backend error-reporting, it's often important to show the misguided
value, because it may be coming from a complex query where it's hard
to find the source of the problematic value. But if the user types
-Z42 or -Zborked, I'm not sure it's important to tell them that the
problem is with "42" or "borked". It's more important to explain the
concept, or such would be my judgement.

Also, consider an option where the value must be an integer between 1
and 100 or one of several fixed strings (e.g. think of
recovery_target_timeline). The user clearly can't use the generic
error message in that case. Perhaps the answer is to say that such
users shouldn't use the provided range-checking function but rather
implement the logic from scratch. But that seems a bit limiting.

Also, suppose the user doesn't want to pg_log_error(). Maybe it's a
warning. Maybe it doesn't even need to be logged.

What this boils down to in the end is that putting more of the policy
decisions into the function helps ensure consistency and save code
when the function is used, but it also results in the function being
used less often. Reasonable people can differ on the merits of
different approaches, but for me the down side of returning the error
message appears minor at most, and the up sides seem significant.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company