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
