Re: libpq stricter integer parsing

Hello Michael,

> On Fri, Sep 07, 2018 at 11:22:14PM +0200, Fabien COELHO wrote:
>> Weird indeed. However, even if the patch applied cleanly for me, it was not
>> compiling anymore. Attached an updated version, in which I also tried to
>> improve the error message on trailing garbage.
>
> At least now it applies for me, thanks.
>
> +    Integer values expected for keywords <literal>port</literal>,
> +    <literal>connect_timeout</literal>,
> +    <literal>keepalives_idle</literal>,
> +    <literal>keepalives_interval</literal> and
> +    <literal>keepalives_timeout</literal> are parsed strictly.

> Wouldn't it be better to actually document what "parsed strictly" means?

Hmmm. This is what the sentence following the above tries to explain 
implicitely:

   Versions of <application>libpq</application> before
   <product>PostgreSQL 12</product> accepted trailing garbage or overflows.

Maybe I can rephrase it in one sentence, eg:

"From PostgreSQL 12, integer values for keywords ... are parsed strictly, 
i.e. trailing garbage and errors on overflows are not accepted anymore."

> A wild though: we already have things like pg_strtoint32 or pg_atoi
> which do similar error handling in smarter ways.  Wouldn't we want to
> refactor the code so as libpq makes use of such routines?  This would
> mean that the error string should be moved around, and allowing
> frontends to use those utilities requires some extra engineering.

I thought of that but rejected it.

The pg_* function you mention are in the backend code, where error 
handling is managed differently, and this function is really only about 
error handling. Moreover "strtol" is already used "libpq/fe-connect.c" for 
the same purpose of parsing int and detecting errors (URL port, keep 
alives).

So the implied changes to try to factor out the functionnality looked like 
a bad idea (where should I put it [probably pgport?]? how should I manage 
errors in a client/server compatible way [no simple idea]?), hence the 
local re-inventation, also to avoid any impact on the backend code.

> Not that this patch should reinvent the world...

Sure.

-- 
Fabien.