Обсуждение: timeout of pg_receivexlog --status-interval

Hi,

At 1047 line of receivelog.c:CopyStreamPoll(), we set NULL to
timeoutptr variable.
if the value of timeoutprt is set NULL then the process will wait
until can read socket using by select() function as following.

    if (timeout_ms < 0)
        timeoutptr = NULL;
    else
    {
        timeout.tv_sec = timeout_ms / 1000L;
timeout.tv_usec = (timeout_ms % 1000L) * 1000L;
        timeoutptr = &timeout;
    }

    ret = select(PQsocket(conn) + 1, &input_mask, NULL, NULL, timeoutptr);

But the 1047 line of receivelog.c is never executed because the value
of timeout_ms is NOT allowed less than 0 at CopyStreamReceive which is
only one function calls CopyStreamPoll().
The currently code, if we specify -s to 0 then CopyStreamPoll()
function is never called.
And the pg_receivexlog will be execute PQgetCopyData() and failed, in
succession.

I think that it is contradiction, and should execute select() function
with NULL of fourth argument.
the attached patch allows to execute select() with NULL, i.g.,
pg_receivexlog.c will wait until can  read socket without timeout, if
-s is specified to 0.

Regards,
-------
Sawada Masahiko

On Thu, Jul 10, 2014 at 11:10 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
> Hi,
>
> At 1047 line of receivelog.c:CopyStreamPoll(), we set NULL to
> timeoutptr variable.
> if the value of timeoutprt is set NULL then the process will wait
> until can read socket using by select() function as following.
>
>     if (timeout_ms < 0)
>         timeoutptr = NULL;
>     else
>     {
>         timeout.tv_sec = timeout_ms / 1000L;
> timeout.tv_usec = (timeout_ms % 1000L) * 1000L;
>         timeoutptr = &timeout;
>     }
>
>     ret = select(PQsocket(conn) + 1, &input_mask, NULL, NULL, timeoutptr);
>
> But the 1047 line of receivelog.c is never executed because the value
> of timeout_ms is NOT allowed less than 0 at CopyStreamReceive which is
> only one function calls CopyStreamPoll().
> The currently code, if we specify -s to 0 then CopyStreamPoll()
> function is never called.
> And the pg_receivexlog will be execute PQgetCopyData() and failed, in
> succession.

Thanks for reporting this! Yep, this is a problem.

> I think that it is contradiction, and should execute select() function
> with NULL of fourth argument.
> the attached patch allows to execute select() with NULL, i.g.,
> pg_receivexlog.c will wait until can  read socket without timeout, if
> -s is specified to 0.

Your patch changed the code so that CopyStreamPoll is called even
when the timeout is 0. I don't agree with this change because the
timeout=0 basically means that the caller doesn't request to block and
there is no need to call CopyStreamPoll in this case. So I'm thinking to
apply the attached patch. Thought?

Regards,

--
Fujii Masao

On Tue, Jul 15, 2014 at 7:38 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Thu, Jul 10, 2014 at 11:10 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>> Hi,
>>
>> At 1047 line of receivelog.c:CopyStreamPoll(), we set NULL to
>> timeoutptr variable.
>> if the value of timeoutprt is set NULL then the process will wait
>> until can read socket using by select() function as following.
>>
>>     if (timeout_ms < 0)
>>         timeoutptr = NULL;
>>     else
>>     {
>>         timeout.tv_sec = timeout_ms / 1000L;
>> timeout.tv_usec = (timeout_ms % 1000L) * 1000L;
>>         timeoutptr = &timeout;
>>     }
>>
>>     ret = select(PQsocket(conn) + 1, &input_mask, NULL, NULL, timeoutptr);
>>
>> But the 1047 line of receivelog.c is never executed because the value
>> of timeout_ms is NOT allowed less than 0 at CopyStreamReceive which is
>> only one function calls CopyStreamPoll().
>> The currently code, if we specify -s to 0 then CopyStreamPoll()
>> function is never called.
>> And the pg_receivexlog will be execute PQgetCopyData() and failed, in
>> succession.
>
> Thanks for reporting this! Yep, this is a problem.
>
>> I think that it is contradiction, and should execute select() function
>> with NULL of fourth argument.
>> the attached patch allows to execute select() with NULL, i.g.,
>> pg_receivexlog.c will wait until can  read socket without timeout, if
>> -s is specified to 0.
>
> Your patch changed the code so that CopyStreamPoll is called even
> when the timeout is 0. I don't agree with this change because the
> timeout=0 basically means that the caller doesn't request to block and
> there is no need to call CopyStreamPoll in this case. So I'm thinking to
> apply the attached patch. Thought?
>

Thank you for the response.
I think this is  better.

One another point about select() function, I think that they are same
behavior between the fifth argument is NULL and 0(i.g. 0 sec).
so I think that it's better to change the CopyStreamPoll() as followings.

@@ -1043,7 +1043,7 @@ CopyStreamPoll(PGconn *conn, long timeout_ms)       FD_ZERO(&input_mask);
FD_SET(PQsocket(conn),&input_mask);
 

-       if (timeout_ms < 0)
+       if (timeout_ms <= 0)               timeoutptr = NULL;       else       {

Please give me feed back.

Regards,

-------
Sawada Masahiko

> >> Hi,
> >>
> >> At 1047 line of receivelog.c:CopyStreamPoll(), we set NULL to
> >> timeoutptr variable.
> >> if the value of timeoutprt is set NULL then the process will wait
> >> until can read socket using by select() function as following.
> >>
> >>     if (timeout_ms < 0)
> >>         timeoutptr = NULL;
> >>     else
> >>     {
> >>         timeout.tv_sec = timeout_ms / 1000L; timeout.tv_usec =
> >> (timeout_ms % 1000L) * 1000L;
> >>         timeoutptr = &timeout;
> >>     }
> >>
> >>     ret = select(PQsocket(conn) + 1, &input_mask, NULL, NULL,
> >> timeoutptr);
> >>
> >> But the 1047 line of receivelog.c is never executed because the value
> >> of timeout_ms is NOT allowed less than 0 at CopyStreamReceive which
> >> is only one function calls CopyStreamPoll().
> >> The currently code, if we specify -s to 0 then CopyStreamPoll()
> >> function is never called.
> >> And the pg_receivexlog will be execute PQgetCopyData() and failed,
> in
> >> succession.
> >
> > Thanks for reporting this! Yep, this is a problem.
> >
> >> I think that it is contradiction, and should execute select()
> >> function with NULL of fourth argument.
> >> the attached patch allows to execute select() with NULL, i.g.,
> >> pg_receivexlog.c will wait until can  read socket without timeout,
> if
> >> -s is specified to 0.
> >
> > Your patch changed the code so that CopyStreamPoll is called even when
> > the timeout is 0. I don't agree with this change because the
> > timeout=0 basically means that the caller doesn't request to block and
> > there is no need to call CopyStreamPoll in this case. So I'm thinking
> > to apply the attached patch. Thought?
> >
> 
> Thank you for the response.
> I think this is  better.
> 
> One another point about select() function, I think that they are same
> behavior between the fifth argument is NULL and 0(i.g. 0 sec).
> so I think that it's better to change the CopyStreamPoll() as followings.
> 
> @@ -1043,7 +1043,7 @@ CopyStreamPoll(PGconn *conn, long timeout_ms)
>         FD_ZERO(&input_mask);
>         FD_SET(PQsocket(conn), &input_mask);
> 
> -       if (timeout_ms < 0)
> +       if (timeout_ms <= 0)
>                 timeoutptr = NULL;
>         else
>         {
> 
> Please give me feed back.

I have no problem with either of the suggestions, if we specify -s to 0.

However, the fix of CopyStreamPoll(), I can't choose the route which doesn't carry out select().

I have proposed a patch that was in reference to walreceiver, 
there is a logic to continuously receive messages as walreceiver in that patch, 
and the route which doesn't carry out select() is necessary for it.

I think that a condition change of CopyStreamReceive() is better from expansibility. Thought?

Regards,

--
Furuya Osamu

On Wed, Jul 16, 2014 at 2:29 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
> On Tue, Jul 15, 2014 at 7:38 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Thu, Jul 10, 2014 at 11:10 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>>> Hi,
>>>
>>> At 1047 line of receivelog.c:CopyStreamPoll(), we set NULL to
>>> timeoutptr variable.
>>> if the value of timeoutprt is set NULL then the process will wait
>>> until can read socket using by select() function as following.
>>>
>>>     if (timeout_ms < 0)
>>>         timeoutptr = NULL;
>>>     else
>>>     {
>>>         timeout.tv_sec = timeout_ms / 1000L;
>>> timeout.tv_usec = (timeout_ms % 1000L) * 1000L;
>>>         timeoutptr = &timeout;
>>>     }
>>>
>>>     ret = select(PQsocket(conn) + 1, &input_mask, NULL, NULL, timeoutptr);
>>>
>>> But the 1047 line of receivelog.c is never executed because the value
>>> of timeout_ms is NOT allowed less than 0 at CopyStreamReceive which is
>>> only one function calls CopyStreamPoll().
>>> The currently code, if we specify -s to 0 then CopyStreamPoll()
>>> function is never called.
>>> And the pg_receivexlog will be execute PQgetCopyData() and failed, in
>>> succession.
>>
>> Thanks for reporting this! Yep, this is a problem.
>>
>>> I think that it is contradiction, and should execute select() function
>>> with NULL of fourth argument.
>>> the attached patch allows to execute select() with NULL, i.g.,
>>> pg_receivexlog.c will wait until can  read socket without timeout, if
>>> -s is specified to 0.
>>
>> Your patch changed the code so that CopyStreamPoll is called even
>> when the timeout is 0. I don't agree with this change because the
>> timeout=0 basically means that the caller doesn't request to block and
>> there is no need to call CopyStreamPoll in this case. So I'm thinking to
>> apply the attached patch. Thought?
>>
>
> Thank you for the response.
> I think this is  better.
>
> One another point about select() function, I think that they are same
> behavior between the fifth argument is NULL and 0(i.g. 0 sec).

No, per manpage of select(2), select(2) with timeout=0 behaves differently
from select(2) with timeout=NULL. So I don't think your suggestion is
acceptable...

Regards,

-- 
Fujii Masao