timeout of pg_receivexlog --status-interval

Started by Sawada Masahikoover 11 years ago5 messages

sawada.mshk@gmail.com

over 11 years ago

1 attachment(s)

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

Attachments:

receivexlog-timout.patchapplication/octet-stream; name=receivexlog-timout.patchDownload

*** a/src/bin/pg_basebackup/receivelog.c
--- b/src/bin/pg_basebackup/receivelog.c
***************
*** 1094,1107 **** CopyStreamReceive(PGconn *conn, long timeout, char **buffer)
  		 * No data available. Wait for some to appear, but not longer than
  		 * the specified timeout, so that we can ping the server.
  		 */
! 		if (timeout > 0)
! 		{
! 			int		ret;
  
! 			ret = CopyStreamPoll(conn, timeout);
! 			if (ret <= 0)
! 				return ret;
! 		}
  
  		/* Else there is actually data on the socket */
  		if (PQconsumeInput(conn) == 0)
--- 1094,1104 ----
  		 * No data available. Wait for some to appear, but not longer than
  		 * the specified timeout, so that we can ping the server.
  		 */
! 		int		ret;
  
! 		ret = CopyStreamPoll(conn, timeout);
! 		if (ret <= 0)
! 			return ret;
  
  		/* Else there is actually data on the socket */
  		if (PQconsumeInput(conn) == 0)

Fujii Masao

masao.fujii@gmail.com

over 11 years ago

In reply to: Sawada Masahiko (#1)

1 attachment(s)

Re: timeout of pg_receivexlog --status-interval

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

Attachments:

bugfix.patchtext/x-patch; charset=US-ASCII; name=bugfix.patchDownload

diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 32afc8d..44b9fcc 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -1094,7 +1094,7 @@ CopyStreamReceive(PGconn *conn, long timeout, char **buffer)
 		 * No data available. Wait for some to appear, but not longer than
 		 * the specified timeout, so that we can ping the server.
 		 */
-		if (timeout > 0)
+		if (timeout != 0)
 		{
 			int		ret;

Sawada Masahiko

sawada.mshk@gmail.com

over 11 years ago

In reply to: Fujii Masao (#2)

Re: timeout of pg_receivexlog --status-interval

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Noname

furuyao@pm.nttdata.co.jp

over 11 years ago

In reply to: Sawada Masahiko (#3)

Re: 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.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Fujii Masao

masao.fujii@gmail.com

over 11 years ago

In reply to: Sawada Masahiko (#3)

Re: timeout of pg_receivexlog --status-interval

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers