Re: Non-blocking communication between a frontend and a backend (pqcomm)

Hi,

http://archives.postgresql.org/pgsql-hackers/2009-07/msg00191.php

In line with Robert's suggestion, I submit non-blocking pqcomm patch
as a self-contained one.

This patch provides support for non-blocking communication between
a frontend and a backend. The upcoming synchronous replication patch
needs this to make walsender send XLOG records and receive a reply
from the standby server concurrently. Specifically, this patch provides
the function to check for a socket by using "select() / poll()", and
the functions to read the data from local buffer instead of the connection.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Fujii Masao wrote:
> http://archives.postgresql.org/pgsql-hackers/2009-07/msg00191.php
>
> In line with Robert's suggestion, I submit non-blocking pqcomm patch
> as a self-contained one.
>

Here's my initial review of the non-blocking pqcomm patch. The patch applies
cleanly and passes regression. Generally looks nice and clean. Couple of remarks
from the department of nitpicking:

* In secure_poll() the handling of timeouts is different depending whether
poll(), select() or SSL_pending() is used. The latter doesn't use the
timeout value at all, and for select() it is impossible to specify indefinite
timeout.
* occasional "blank" lines consisting of a single tab character -- maybe
a left-over from editor auto-indent. Not sure of how much a problem this
is, given that the blanks will be removed by pg_indent.
* Comment on pq_wait() seems to have a typo: "-1 if an error directly."

I have done limited testing on Linux i686 (HAVE_POLL only) -- the non-blocking
functions behave as expected.

regards,
Martin

On Fri, Jul 17, 2009 at 5:26 PM, Martin Pihlak<martin(dot)pihlak(at)gmail(dot)com> wrote:
> Fujii Masao wrote:
>> http://archives.postgresql.org/pgsql-hackers/2009-07/msg00191.php
>>
>> In line with Robert's suggestion, I submit non-blocking pqcomm patch
>> as a self-contained one.
>>
>
> Here's my initial review of the non-blocking pqcomm patch. The patch applies
> cleanly and passes regression. Generally looks nice and clean. Couple of remarks
> from the department of nitpicking:
>
> * In secure_poll() the handling of timeouts is different depending whether
>  poll(), select() or SSL_pending() is used. The latter doesn't use the
>  timeout value at all, and for select() it is impossible to specify indefinite
>  timeout.
> * occasional "blank" lines consisting of a single tab character -- maybe
>  a left-over from editor auto-indent. Not sure of how much a problem this
>  is, given that the blanks will be removed by pg_indent.
> * Comment on pq_wait() seems to have a typo: "-1 if an error directly."
>
> I have done limited testing on Linux i686 (HAVE_POLL only) -- the non-blocking
> functions behave as expected.

Fujii Masao,

Are you planning to update this patch based on Martin's review?

...Robert

Hi,

On Wed, Jul 22, 2009 at 2:20 AM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
> Fujii Masao,
>
> Are you planning to update this patch based on Martin's review?

Sure. Attached is an updated patch.

> On Fri, Jul 17, 2009 at 5:26 PM, Martin Pihlak<martin(dot)pihlak(at)gmail(dot)com> wrote:
>> Here's my initial review of the non-blocking pqcomm patch. The patch applies
>> cleanly and passes regression. Generally looks nice and clean. Couple of remarks
>> from the department of nitpicking:

Thanks for reviewing the patch!

>> * In secure_poll() the handling of timeouts is different depending whether
>>  poll(), select() or SSL_pending() is used. The latter doesn't use the
>>  timeout value at all, and for select() it is impossible to specify indefinite
>>  timeout.

Fixed. I tweaked the handling of the fifth argument 'timeout' of select(); when
a negative number is specified to a timeout of secure_poll(), NULL is set to
that 'timeout', which can block select() indefinitely.

Since SSL_pending() doesn't wait for data to arrive (i.e., doesn't use timeout),
I didn't change the code related to that function.

>> * occasional "blank" lines consisting of a single tab character -- maybe
>>  a left-over from editor auto-indent. Not sure of how much a problem this
>>  is, given that the blanks will be removed by pg_indent.

Fixed.

>> * Comment on pq_wait() seems to have a typo: "-1 if an error directly."

Fixed.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> On Wed, Jul 22, 2009 at 2:20 AM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>> Are you planning to update this patch based on Martin's review?

> Sure. Attached is an updated patch.

I looked at this patch. I don't see how we can consider accepting it
by itself. It adds a bunch of code that is not used anywhere and hence
can't be tested, in service of goals explained nowhere, but presumably
part of some other patch that hasn't been reviewed and might or might
not get accepted when it is presented. The only thing that's really
clear is that it pokes holes in the abstraction (such as it is)
presented by pqcomm.c.

The reason I want to see the calling code is that I doubt this is a very
useful API extension as-is. I can see the point of probing to see if
any more bytes are available, but it's not clear that there is a reason
to collect only part of a message once the client has sent one. I am
also thinking that if you do need the ability to get control back
without blocking on the socket, you probably will need that for writes
as well as reads; and this patch doesn't cover the write case.

I think you should just submit this with the code that uses it, so we
can evaluate whether the overall concept is a good one or not.

regards, tom lane

I wrote:
> I am also thinking that if you do need the ability to get control back
> without blocking on the socket, you probably will need that for writes
> as well as reads; and this patch doesn't cover the write case.

Oh, another gripe: I'll bet a nickel that this doesn't work very nicely
under SSL. Bytes available on the socket doesn't necessarily equate to
decrypted payload bytes being available. Depending on how you're using
secure_poll, that might be okay, but it seems like a hazard waiting to
trap unwary maintainers.

regards, tom lane

On Fri, Jul 24, 2009 at 7:21 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>> On Wed, Jul 22, 2009 at 2:20 AM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>>> Are you planning to update this patch based on Martin's review?
>
>> Sure. Attached is an updated patch.
>
> I looked at this patch.  I don't see how we can consider accepting it
> by itself.  It adds a bunch of code that is not used anywhere and hence
> can't be tested, in service of goals explained nowhere, but presumably
> part of some other patch that hasn't been reviewed and might or might
> not get accepted when it is presented.  The only thing that's really
> clear is that it pokes holes in the abstraction (such as it is)
> presented by pqcomm.c.
>
> The reason I want to see the calling code is that I doubt this is a very
> useful API extension as-is.  I can see the point of probing to see if
> any more bytes are available, but it's not clear that there is a reason
> to collect only part of a message once the client has sent one.  I am
> also thinking that if you do need the ability to get control back
> without blocking on the socket, you probably will need that for writes
> as well as reads; and this patch doesn't cover the write case.
>
> I think you should just submit this with the code that uses it, so we
> can evaluate whether the overall concept is a good one or not.

This was split out from Synch Rep based on my suggestion to submit
separately any parts that are separately committable, but that doesn't
seem to be the case given your comments here. I guess the question is
whether it's necessary and/or desirable to put in the effort to create
a general-purpose facility, or whether we should be satisfied with the
minimum level of infrastructure necessary to support Synch Rep and
just incorporate it into that patch.

Thoughts?

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Jul 24, 2009 at 7:21 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I think you should just submit this with the code that uses it, so we
>> can evaluate whether the overall concept is a good one or not.

> This was split out from Synch Rep based on my suggestion to submit
> separately any parts that are separately committable, but that doesn't
> seem to be the case given your comments here. I guess the question is
> whether it's necessary and/or desirable to put in the effort to create
> a general-purpose facility, or whether we should be satisfied with the
> minimum level of infrastructure necessary to support Synch Rep and
> just incorporate it into that patch.

General-purpose facility *for what*? It's impossible to evaluate the
code without a definition of the purpose behind it.

What I actually think should come first is a spec for the client
protocol this is intended to support. It's not apparent to me at
the moment why the backend should need non-blocking read at all.

regards, tom lane

On Sat, Jul 25, 2009 at 11:41 AM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Jul 24, 2009 at 7:21 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I think you should just submit this with the code that uses it, so we
>>> can evaluate whether the overall concept is a good one or not.
>
>> This was split out from Synch Rep based on my suggestion to submit
>> separately any parts that are separately committable, but that doesn't
>> seem to be the case given your comments here.  I guess the question is
>> whether it's necessary and/or desirable to put in the effort to create
>> a general-purpose facility, or whether we should be satisfied with the
>> minimum level of infrastructure necessary to support Synch Rep and
>> just incorporate it into that patch.
>
> General-purpose facility *for what*?  It's impossible to evaluate the
> code without a definition of the purpose behind it.
>
> What I actually think should come first is a spec for the client
> protocol this is intended to support.  It's not apparent to me at
> the moment why the backend should need non-blocking read at all.

[ reads the patch ]

OK, I agree, I can't see what this is for either from the code that is
here. I think I read a little more meaning into the title of the
patch than was actually there. It seems like the appropriate thing to
do is mark this returned with feedback, so I'm going to go do that.

...Robert

Hi,

On Sun, Jul 26, 2009 at 6:42 AM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
> OK, I agree, I can't see what this is for either from the code that is
> here.  I think I read a little more meaning into the title of the
> patch than was actually there.  It seems like the appropriate thing to
> do is mark this returned with feedback, so I'm going to go do that.

OK. I'll change and resubmit the patch with Synch Rep which uses it
in the next CommitFest.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Hi,

On Sat, Jul 25, 2009 at 8:39 AM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Oh, another gripe: I'll bet a nickel that this doesn't work very nicely
> under SSL.  Bytes available on the socket doesn't necessarily equate to
> decrypted payload bytes being available.  Depending on how you're using
> secure_poll, that might be okay, but it seems like a hazard waiting to
> trap unwary maintainers.

Is it only necessary to add the comment about how to use secure_poll?

There is the assumption that secure_poll must be used with secure_write/read
(e.g., in read case, pq_recvbuf instead of native recv should be called after
passing pq_wait). So, it's assumed that encrypted data are resolved in those
R/W functions and only decrypted data are located in buffer.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> On Sat, Jul 25, 2009 at 8:39 AM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Oh, another gripe: I'll bet a nickel that this doesn't work very nicely
>> under SSL. Bytes available on the socket doesn't necessarily equate to
>> decrypted payload bytes being available. Depending on how you're using
>> secure_poll, that might be okay, but it seems like a hazard waiting to
>> trap unwary maintainers.

> Is it only necessary to add the comment about how to use secure_poll?

> There is the assumption that secure_poll must be used with secure_write/read
> (e.g., in read case, pq_recvbuf instead of native recv should be called after
> passing pq_wait). So, it's assumed that encrypted data are resolved in those
> R/W functions and only decrypted data are located in buffer.

Well, actually, this description perfectly illustrates my basic
complaint: the patch breaks the API abstraction provided by pqcomm.c.
Callers are encouraged/forced to deal with the next layer down, and to
the extent that pqcomm.c does anything useful, callers have to allow for
that too.

As far as the read side of pq_wait goes, it should probably be more
like
* return true if any bytes are in the buffer
* else try to read some bytes into the buffer, without blocking
* now return true or false depending on whether bytes are in
the buffer.
(Or perhaps instead of true/false, return the count of available bytes.)
Also, I suspect the API needs to make a distinction between "no more
bytes available yet" and EOF (channel closed, so no more bytes ever
will be available).

I'm not sure about the write side. The patch isn't really addressing
blocking-on-write, except in the offhand way of having a forWrite option
in pq_wait, but that doesn't seem too well thought out to me. Again,
the buffering pqcomm does makes a direct query of the underlying state
seem pretty suspicious.

regards, tom lane

Hi,

Thanks for the comment!

On Tue, Jul 28, 2009 at 12:27 AM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Well, actually, this description perfectly illustrates my basic
> complaint: the patch breaks the API abstraction provided by pqcomm.c.
> Callers are encouraged/forced to deal with the next layer down, and to
> the extent that pqcomm.c does anything useful, callers have to allow for
> that too.
>
> As far as the read side of pq_wait goes, it should probably be more
> like
> * return true if any bytes are in the buffer
> * else try to read some bytes into the buffer, without blocking
> * now return true or false depending on whether bytes are in
> the buffer.
> (Or perhaps instead of true/false, return the count of available bytes.)

Seems good. But when we wait for 4 bytes and there is 1 byte in the buffer,
the above pq_wait always returns immediately, and we cannot get 4 bytes.
So, I think that we should make pq_wait monitor only the socket. And, we
should make the newly-introduced low-level I/O functions (such as
pq_getbufbytes) load some bytes into the buffer without blocking (i.e.,
pq_wait with no timeout is called by those functions), and return true/false
according to whether the specified bytes are in there. What is your opinion?

> Also, I suspect the API needs to make a distinction between "no more
> bytes available yet" and EOF (channel closed, so no more bytes ever
> will be available).

That's right.

> I'm not sure about the write side. The patch isn't really addressing
> blocking-on-write, except in the offhand way of having a forWrite option
> in pq_wait, but that doesn't seem too well thought out to me.

I'm inclined to use the write side in Synch Rep. So, I'll consider it carefully.

> Again,
> the buffering pqcomm does makes a direct query of the underlying state
> seem pretty suspicious.

Sorry, I didn't get your point. Your concern is only the receiving of a query
from the frontend?

I'm not planning to change the existing communication of a query and result
between the backend and frontend. Non-blocking communication is used in
log-shipping between walsender and walreceiver. In order to speed up log-
shipping, the walsender should send the XLOG records and receive the
reply from walreceiver as concurrently as possible. In other words, I'd like to
prevent the pause that the walsender cannot send the outstanding records
during receiving with blocking. That pause might take long because of slow
network.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center