From: | Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> |
---|---|
To: | Steve Singer <ssinger_pg(at)sympatico(dot)ca> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: libpq SSL with non-blocking sockets |
Date: | 2011-06-28 18:14:44 |
Message-ID: | 4E0A1A14.2070709@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 06/25/2011 12:14 AM, Steve Singer wrote:
> I'm not a libpq guru but I've given your patch a look over.
>
Thanks for the review!
I have since simplified the patch to assume that partial SSL writes are
disabled -- according to SSL_write(3) this is the default behaviour.
Now the SSL retry buffer only holds the data to be retried, the
remainder is moved to the new outBuffer.
> -The patch doesn't compile with non-ssl builds, the debug at the bottom
> of PQSendSome isn't in an #ifdef
>
Ack, there was another one in pqFlush. Fixed that.
> -I don't think your handling the return code properly. Consider this case.
>
> pqSendSome(some data)
> sslRetryBuf = some Data
> return 1
> pqSendSome(more data)
> it sends all of 'some data'
> returns 0
>
> I think 1 should be returned because all of 'more data' still needs to
> be sent. I think returning a 0 will break PQsetnonblocking if you call
> it when there is data in both sslRetryBuf and outputBuffer.
Hmm, I thought I thought about that. There was a check in the original
patch: "if (conn->sslRetryBytes || (conn->outCount - remaining) > 0)"
So if the SSL retry buffer was emptied it would return 1 if there was
something left in the regular output buffer. Or did I miss something
there?
> We might even want to try sending the data in outputBuffer after we've
> sent all the data sitting in sslRetryBuf.
>
IMHO that'd add unnecessary complexity to the pqSendSome. As this only
happens in non-blocking mode the caller should be well prepared to
handle the retry.
> If you close the connection with an outstanding sslRetryBuf you need to
> free it.
Fixed.
New version of the patch attached.
regards,
Martin
Attachment | Content-Type | Size |
---|---|---|
ssl-write-retry-2.patch | text/x-patch | 4.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Heikki Linnakangas | 2011-06-28 18:29:35 | Re: SSI modularity questions |
Previous Message | Alex Hunsaker | 2011-06-28 18:13:36 | Re: add support for logging current role (what to review?) |