Re: Async branch documentation and review

On 08/04/10 14:33, Daniele Varrazzo wrote:
> Hello,
>
> I've written documentation for the new Psycopg async support. Together
> with other patches it is available in the "fix22" branch of my git
> repos:

Hi, and thanks a lot for the documentation and the review.

> Jan, please tell me if everything is ok or if there is anything to be fixed.

> Do we really need cursor.poll()? There can only be a single async
> cursor in execution per connection. On the other side we can't do
> without connection.poll() because is using during connection, when
> there is no cursor. What about dropping cursor.poll() and only leave
> connection.poll()? If an app only had a reference to the cursor it
> would always be possible to call curs.connection.poll().

I don't mind having only connection.poll, although it feels a bit 
strange to me. If we go that way it's one step short of having (in async 
mode) only connection.execute() and getting rid of the cursors 
whatsoever (which is not necessarily bad).
Another thing is that if/when we start supporting async named cursors, 
we might need to still call something on the cursor (haven't thought 
about the interface yet).

In short, I wouldn't personally go for removing poll() from the cursor, 
but I don't have anything against it. Remember, that these functions are 
not really meant to be used directly and it is expected that some kind 
of framework will take care of not executing two async queries 
simultaneously and polling the right thing.

> Do we really need cursor.fileno()? This is stronger than the above,
> because while conceptually in the cursor there may be state that the
> connection doesn't know about (not in the current implementation, but
> there may be), there is honestly a single fd, and it belongs to the
> connection.

Same as above, no problem with having only connection.fileno() (and no 
harm in keeping it).

> Jan, looking at your example in the test_async, the procedure to
> obtain a nonblocking connection seems being:
 >
 >  [complicated code]
>
> I understand why you start with a POLL_WRITE, have a write wait and
> then poll, but only because I've read the libpq documentation [1] and
> dealt with this quirk myself in the green branch. I think it would be

> Wouldn't it be better to have connection.poll() return a consistent
> result and so suggest the user a single wait loop for both the phases?

This has one problem, namely that if you don't wait for the socket to 
become writable you can't reliably even start executing any query, 
because if the write buffer happens to be full you will get an 
EWOULDBLOCK. The same goes for a connection attempt, calling 
PQconnectionPoll will return an error if the write socket is full (which 
will almost never happen, but for execute() it's not that obvious).

So you always need that kind of logic:

  * wait for fd to be writable
  * execute query or start connecting
  * poll and wait on the read or write event until it's done

You could make poll() return POLL_WRITE by default, but then the user 
would have to first do select once, then execute a query and then enter 
the select loop. I can't see a good way to prevent that.

> conn.executing() returns True only when a cursor is executing, not
> when there is an async connection attempt. I suggest either to make it
> return True on connection() too or to move it on the cursor for
> consistency with the cursor.poll() being executed (my preference is to
> only have connection.poll() as stated above).

+1 to return True from executing() if there is an async connection 
attempt underway (I don't really expect that function to be used by many 
things except unit tests), it's more logical.

> I just discovered another quirk in having a cursor.poll() while the
> responsibility of the communication state really belongs to the
> connection: cursors can steal results each other:

:( Yeah, that kind of sucks. Could this fall in the category of "so 
don't do it"? Or should we add an exception when you try to poll() a 
cursor that's not the one currently executing?

> async and connection_factory don't play well together: I added a test
> that fails (http://tinyurl.com/yk6hp5d) to the test suite.

Yeah, thanks for cleaning this up.

And thanks again for reviewing these interfaces + the docs.

Cheers,
Jan