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.
