On Thu, Oct 17, 2019 at 10:10:17PM +0200, Lars Kanis wrote:
> That's why I changed connectDBComplete() only, instead of setting the
> status directly in parse_int_param().
Yes, you shouldn't do that as the keepalive parameters and
tcp_user_timeout have some specific handling when it comes to defaults
depending on the platform and we have some retry logic when specifying
multiple hosts.
Now, there is actually more to it than it looks at first glance. Your
patch is pointing out at a failure within the regression tests of the
ECPG driver, as any parameters part of a connection string may have
trailing spaces which are considered as incorrect by the patch,
causing the connection to fail.
In short, on HEAD this succeeds but would fail with your patch:
$ psql 'postgresql:///postgres?host=/tmp&connect_timeout=14 &port=5432'
psql: error: could not connect to server: invalid integer value "14 "
for connection option "connect_timeout"
Parameter names are more restrictive, as URLs don't allow leading or
trailing spaces for them. On HEAD, we allow leading spaces for
integer parameters as the parsing uses strtol(3), but not for the
trailing spaces, which is a bit crazy and I think that we had better
not break that if the parameter value correctly defines a proper
integer. So attached is a patch to skip trailing whitespaces as well,
which also fixes the issue with ECPG. I have refactored the parsing
logic a bit while on it. The comment at the top of parse_int_param()
needs to be reworked a bit more.
We could add some TAP tests for that, but I don't see a good area to
check after connection parameters. We have tests for multi-host
strings in 001_stream_rep.pl but that already feels misplaced as those
tests are for recovery. Perhaps we could add directly regression
tests for libpq. I'll start a new thread about that once we are done
here, the topic is larger.
(Note to self: Ed Morley needs to be credited for the report as well.)
--
Michael