(2012/05/01 0:30), Ryan Kelly wrote:
> On Mon, Apr 30, 2012 at 09:02:33AM -0400, Alvaro Herrera wrote:
>> Well, do *you* want it?
> Of course. That way I can stop patching my psql and go back to using the
> one that came with my release :)
Here is result of my review of v4 patch.
Submission
----------
- The patch is in context diff format
- It needs rebase for HEAD, but patch command takes care automatically.
- Make finishes cleanly, and all regression tests pass
Functionality
-------------
After applying this patch, I could cancel connection attempt at start-up
by pressing Ctrl+C on terminal just after invoking psql command with an
unused IP address. Without this patch, such attempt ends up with error
such as "No route to host" after uninterruptable few seconds (maybe the
duration until error would depend on environment).
Connection attempt by \connect command could be also canceled by
pressing Ctrl+C on psql prompt.
In addition, I tried setting PGCONNECT_TIMEOUT to 0 (infinite), but psql
gave up after few seconds, for both start-up and re-connect. Is this
intentional behavior?
Detail of changes
-----------------
Basically this patch consists of three changes:
1) defer setup of cancel handler until start-up connection has established
2) new libpq API PQconnectTimeout which returns connect_timeout value of
current session
3) use asynchronous connection API at \connect psql command, this
requires API added by 2)
These seem reasonable to achieve canceling connection attempt at
start-up and \connect, but, as Ryan says, codes added to do_connect
might need more simplification.
I have some random comments.
- Checking status by calling PQstatus just after PQconnectStartParams is
necessary.
- Copying only "select()" part of pqSocketPoll seems not enough, should
we use poll(2) if it is supported?
- Don't we need to clear error message stored in PGconn after
PQconnectPoll returns OK status, like connectDBComplete?
Regards,
--
Shigeru HANADA
