Re: Change atoi to strtol in same place

On Mon, 7 Oct 2019 at 13:21, Joe Nelson <joe(at)begriffs(dot)com> wrote:
>
> Ashutosh Sharma wrote:
> > One suggestion: The start value for port number is set to 1, however
> > it seems like the port number that falls in the range of 1-1023 is
> > reserved and can't be used. So, is it possible to have the start value
> > as 1024 instead of 1 ?
>
> Good idea -- changed it. I also created macros FE_UTILS_PORT_{MIN,MAX}
> so the range can be updated in one place for all utilities.

(I've only had a very quick look at this, and FWIW, here's my opinion)

It's not for this patch to decide what ports clients can connect to
PostgreSQL on. As far as I'm aware Windows has no restrictions on what
ports unprivileged users can listen on. I think we're likely going to
upset a handful of people if we block the client tools from connecting
to ports < 1024.

> > Further, I encountered one syntax error (INT_MAX undeclared) as the
> > header file "limits.h" has not been included in postgres_fe.h or
> > option.h
>
> Oops. Added limits.h now in option.h. The Postgres build accidentally
> worked on my system without explicitly including this header because
> __has_builtin(__builtin_isinf) is true for me so src/include/port.h
> pulled in math.h with an #if which pulled in limits.h.
>
> > Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> > > The wording is a bit strange. How about something like pg_standy:
> > > invalid argument to -k: %s
>
> I updated the error messages in pg_standby.
>
> > > > *error = psprintf(_("could not parse '%s' as integer"), str);
> > >
> > > ... except that they would rather be more explicit about what the
> > > problem is. "insufficient digits" or "extraneous character", etc.

This part seems over-complex to me. What's wrong with just returning a
bool and if the caller gets "false", then just have it emit the error,
such as:

"compression level must be between %d and %d"

I see Michael's patch is adding this new return type, but really, is
there a good reason why we need to do something special when the user
does not pass in an integer?

Current patch:
$ pg_dump -Z blah
invalid compression level: could not parse 'blah' as integer

I propose:
$ pg_dump -Z blah
compression level must be an integer in range 0..9

This might save a few round trips, e.g the current patch will do:
$ pg_dump -Z blah
invalid compression level: could not parse 'blah' as integer
$ pg_dump -Z 12345
invalid compression level: 12345 is outside range 0..9
$ ...

Also:

+ case PG_STRTOINT_RANGE_ERROR:
+ *error = psprintf(_("%s is outside range "
+ INT64_FORMAT ".." INT64_FORMAT),
+ str, min, max);

The translation string here must be consistent over all platforms. I
think this will cause issues if the translation string uses %ld and
the platform requires %lld?

I think what this patch should be really aiming for is to simplify the
client command-line argument parsing and adding what benefits it can.
I don't think there's really a need to make anything more complex than
it is already here.

I think you should maybe aim for 2 patches here.

Patch 1: Add new function to validate range and return bool indicating
if the string is an integer within range. Set output argument to the
int value if it is valid. Modify all locations where we currently
validate the range of the input arg to use the new function.

Patch 2: Add additional validation where we don't currently do
anything. e.g pg_dump -j

We can then see if there's any merit in patch 2 of if it's adding more
complexity than is really needed.

I also think some compilers won't like:

+ compressLevel = parsed;

given that "parsed" is int64 and "compressLevel" is int, surely some
compilers will warn of possible truncation? An explicit cast to int
should fix those or you could consider just writing a version of the
function for int32 and int64 and directly passing in the variable to
be set.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services