Re: Change atoi to strtol in same place - Mailing list pgsql-hackers

David Rowley wrote:
> 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.

Makes sense. We can instead allow any port number and if it errors at
connection time then the user will find out at that point.

> 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?

Displaying the range when given a non-integer input could be misleading.
For instance, suppose we put as little restriction on the port number
range as possible, enforcing only that it's positive, between 1 and
INT_MAX.  If someone passes a non-integer value, they would get a
message like:

    invalid port number: must be an integer in range 1..2147483647

Sure, the parsing code will accept such a big number, but we don't want
to *suggest* the number. Notice if the user had passed a well-formed
number for the port it's unlikely to be greater than INT_MAX, so they
wouldn't have to see this weird message.

Perhaps you weren't suggesting to remove the upper limit from port
checking, just to change the lower limit from 1024 back to 1. In that
case we could keep it capped at 65535 and the error message above would
be OK.

Other utilities do have command line args that are allowed the whole
non-negative (but signed) int range, and their error message would show
the big number. It's not misleading in that case, but a little
ostentatious.

> 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:

I do see your reasoning that we're teasing people with a puzzle they
have to solve with multiple invocations. On the other hand, passing a
non-number for the compression level is pretty strange, and perhaps
explicitly calling out the mistake might make someone look more
carefully at what they -- or their script -- is doing.

> 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?

A very good and subtle point. I'll change it to %lld so that a single
format string will work everywhere.

> I think you should maybe aim for 2 patches here.
> 
> Patch 1: ...
> 
> 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.

Are you saying that my current patch adds extra constraints for
pg_dump's -j argument, or that in the future we could do that? Because I
don't believe the current patch adds any appreciable complexity for that
particular argument, other than ensuring the value is positive, which
doesn't seem too contentious.

Maybe we can see whether we can reach consensus on the current
parse-and-check combo patch, and if discussion drags on much longer then
try to split it up?

> 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

Good point, I bet some compilers (justly) warn about truncation. We've
checked the range so I'll add an explicit cast.

> or you could consider just writing a version of the function for int32
> and int64 and directly passing in the variable to be set.

One complication is that the destination values are often int rather
than int32, and I don't know their width in general (probably 32, maybe
16, but *possibly* 64?).  The pg_strtoint64_range() function with range
argument of INT_MAX is flexible enough to handle whatever situation we
encounter. Am I overthinking this part?

-- 
Joe Nelson      https://begriffs.com