Re: implementing asynchronous notifications

On Mon, 11 Apr 2005, Oliver Jowett wrote:

> Andras Kadinger wrote:
> > On Mon, 11 Apr 2005, Andras Kadinger wrote:
> >
> >>I am unclear as to how to handle possible protocol errors (e.g. when what
> >>we end up reading from the connection is not an 'A'sync Notify).
> >>Theoretically, in a working connection this should not happen though.
> >
> > Yes, it could: reading the PostgreSQL protocol documentation, it says
> > "frontends should always be prepared to accept and display NoticeResponse
> > messages, even when the connection is nominally idle".
> >
> > So I now added code to process Error 'N'otifications as well.
>
> You also need to handle errors ('E'). Try shutting down a postmaster (-m
> fast) while idle connections are around -- they'll get spontaneous FATAL
> errors.

Are you certain? The protocol documentations specifically mentions this
case, saying it would send a NoticeResponse:

"It is possible for NoticeResponse messages to be generated due to outside
activity; for example, if the database administrator commands a "fast"
database shutdown, the backend will send a NoticeResponse indicating this
fact before closing the connection. Accordingly, frontends should always
be prepared to accept and display NoticeResponse messages, even when the
connection is nominally idle." -
http://www.postgresql.org/docs/8.0/static/protocol-flow.html#PROTOCOL-ASYNC

Still, I took your word on this now, and added code to handle 'E's.

> > + try {
> > + executor.processNotifies();
> > + } catch (SQLException e) {};
>
> Don't eat the exceptions, let them propagate.

The meaning behind the words "Proof of concept". :)

> (ugh, getNotifications() does not throw SQLException. We should probably
> change that..)

Agreed. I just wasn't sure changing public interfaces was the most polite
way of introducing myself. :)

Fixed now.

> > + while (protoConnection.getTransactionState() == ProtocolConnection.TRANSACTION_IDLE && pgStream.getSocket().getInputStream().available()>0) {
>
> Can you move that reference following into a method on PGStream?
> (hasMessagePending() or something)

Sure! Good idea! Done!

> The test on transaction state is a bit misleading since the connection's
> transaction state should never change inside the loop. Perhaps making
> that a separate test would be clearer.

Done!

> I'm not sure if available() is guaranteed to work on a socket stream
> everywhere (it works fine here, though), but I suppose that at worst you
> get the existing behaviour where you need to send a query.

Same here (the rationale behind my first post). I have read somewhere,
available() might not work with SSLSockets (haven't looked behind that).

> Otherwise, seems fine!

Thank you! :)

Any further comments/improvements?

If none, then hereby I'd like to submit this for inclusion.

Andras