Re: [HACKERS] pg_receivexlog add synchronous mode

2014-08-12 Thread Michael Paquier
On Fri, Aug 8, 2014 at 5:10 PM, Fujii Masao masao.fu...@gmail.com wrote:
 Thanks for the review! Applied the patch.
I noticed that the tab padding for the new option -F in the getops
switch is incorrect. Attached patch corrects that. pgindent would have
caught that anyway, but it doesn't hurt to be correct now.
Thanks,
-- 
Michael
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index 0b7af54..4483c87 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -441,15 +441,15 @@ main(int argc, char **argv)
 			case 'n':
 noloop = 1;
 break;
-		case 'F':
-			fsync_interval = atoi(optarg) * 1000;
-			if (fsync_interval  -1000)
-			{
-fprintf(stderr, _(%s: invalid fsync interval \%s\\n),
-		progname, optarg);
-exit(1);
-			}
-			break;
+			case 'F':
+fsync_interval = atoi(optarg) * 1000;
+if (fsync_interval  -1000)
+{
+	fprintf(stderr, _(%s: invalid fsync interval \%s\\n),
+			progname, optarg);
+	exit(1);
+}
+break;
 			case 'v':
 verbose++;
 break;

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-08-12 Thread Fujii Masao
On Tue, Aug 12, 2014 at 4:34 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Fri, Aug 8, 2014 at 5:10 PM, Fujii Masao masao.fu...@gmail.com wrote:
 Thanks for the review! Applied the patch.
 I noticed that the tab padding for the new option -F in the getops
 switch is incorrect. Attached patch corrects that. pgindent would have
 caught that anyway, but it doesn't hurt to be correct now.

Yeah, that's a problem. But, as you clearly said, upcoming pgindent
would fix this kind of problem. So I'm not sure if it's really worth fixing
only this case independently.

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-08-08 Thread Fujii Masao
On Thu, Aug 7, 2014 at 5:24 PM,  furu...@pm.nttdata.co.jp wrote:
 Okay, applied the patch.

 I heavily modified your patch based on the master that the refactoring
 patch has been applied. Attached is that modified version. Could you
 review that?

 Thank you for the patch.
 I did a review of the patch.

 No problem in the patch.

Thanks for the review! Applied the patch.

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-08-07 Thread furuyao
 Okay, applied the patch.
 
 I heavily modified your patch based on the master that the refactoring
 patch has been applied. Attached is that modified version. Could you
 review that?

Thank you for the patch.
I did a review of the patch. 

No problem in the patch. 

Behavior after the true return of ProcessXLogDataMsg was changed by the patch.
Although it was moving to while(1), it has changed so that a while(r != 0) loop 
may be continued.
Since still_sending is false, although skip processing is performed, a result 
of operation does not change.

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-08-06 Thread Fujii Masao
On Wed, Aug 6, 2014 at 2:34 PM,  furu...@pm.nttdata.co.jp wrote:
  I have improved the patch  by making following changes:
 
  1. Since stream_stop() was redundant, stream_stop() at the time of
 WAL file closing was deleted.
 
  2. Change the Flash judging timing for the readability of source code.
 I have changed the Flash judging timing , from the continuous
 message after receiving to
 before the feedbackmassege decision of continue statement after
 execution.
 
  Thanks for the updated version of the patch!
 
  While reviewing the patch, I found that HandleCopyStream() is still
  long and which decreases the readability of the source code.
  So I feel inclined to refactor the HandleCopyStream() more for better
  readability. What about the attached refactoring patch?

 Sorry, I forgot to attached the patch in previous email. So attached.

 Thank you for the refactoring patch.
 I did a review of the patch.

 -   break;  /* ignore the rest of 
 this XLogData packet */

 +   return true;/* ignore the rest of this 
 XLogData packet */

 For break statement at close of wal file, it is a return to true.
 It may be a behavior of continue statement. Is it satisfactory?

Sorry I failed to see your point.

In the original code, when we reach the end of WAL file and it's streaming
stopping point, we break out of inner loop in the code block for processing
XLogData packet. And then we goes back to top of outer loop in
HandleCopyStream. ISTM that the refactored code also works the same way.
Anyway, could you elaborate the problem?

 The walreceiver distributes XLogWalRcvProcessMsg and XLogWalRcvWrite, but 
 isn't that division necessary?

Not necessary, but I have no objection to the idea.

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-08-06 Thread furuyao
  -   break;  /* ignore
 the rest of this XLogData packet */
 
  +   return true;/* ignore the rest of
 this XLogData packet */
 
  For break statement at close of wal file, it is a return to true.
  It may be a behavior of continue statement. Is it satisfactory?
 
 Sorry I failed to see your point.
 
 In the original code, when we reach the end of WAL file and it's streaming
 stopping point, we break out of inner loop in the code block for
 processing XLogData packet. And then we goes back to top of outer loop
 in HandleCopyStream. ISTM that the refactored code also works the same
 way.
 Anyway, could you elaborate the problem?

I'm sorry. I was confused with the patch that I have to offer. 
It is necessary to exit the loop since the loop added to the continuously 
received the message if the patch. 
Refactor patch is no problem with the contents of the presentation.

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-08-06 Thread Fujii Masao
On Wed, Aug 6, 2014 at 5:10 PM,  furu...@pm.nttdata.co.jp wrote:
  -   break;  /* ignore
 the rest of this XLogData packet */
 
  +   return true;/* ignore the rest of
 this XLogData packet */
 
  For break statement at close of wal file, it is a return to true.
  It may be a behavior of continue statement. Is it satisfactory?

 Sorry I failed to see your point.

 In the original code, when we reach the end of WAL file and it's streaming
 stopping point, we break out of inner loop in the code block for
 processing XLogData packet. And then we goes back to top of outer loop
 in HandleCopyStream. ISTM that the refactored code also works the same
 way.
 Anyway, could you elaborate the problem?

 I'm sorry. I was confused with the patch that I have to offer.
 It is necessary to exit the loop since the loop added to the continuously 
 received the message if the patch.
 Refactor patch is no problem with the contents of the presentation.

Okay, applied the patch.

I heavily modified your patch based on the master that the refactoring
patch has been applied. Attached is that modified version. Could you
review that?

Regards,

-- 
Fujii Masao
diff --git a/doc/src/sgml/ref/pg_receivexlog.sgml b/doc/src/sgml/ref/pg_receivexlog.sgml
index 7c50b01..c15776f 100644
--- a/doc/src/sgml/ref/pg_receivexlog.sgml
+++ b/doc/src/sgml/ref/pg_receivexlog.sgml
@@ -106,6 +106,21 @@ PostgreSQL documentation
  /varlistentry
 
  varlistentry
+   termoption-F replaceable class=parameterinterval/replaceable/option/term
+   termoption--fsync-interval=replaceable class=parameterinterval/replaceable/option/term
+   listitem
+para
+Specifies the maximum time to issue sync commands to ensure the
+received WAL file is safely flushed to disk, in seconds. The default
+value is zero, which disables issuing fsyncs except when WAL file is
+closed. If literal-1/literal is specified, WAL file is flushed as
+soon as possible, that is, as soon as there are WAL data which has
+not been flushed yet.
+/para
+   /listitem
+  /varlistentry
+
+ varlistentry
   termoption-v/option/term
   termoption--verbose/option/term
   listitem
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 5df2eb8..0b02c4c 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -371,7 +371,7 @@ LogStreamerMain(logstreamer_param *param)
 	if (!ReceiveXlogStream(param-bgconn, param-startptr, param-timeline,
 		   param-sysidentifier, param-xlogdir,
 		   reached_end_position, standby_message_timeout,
-		   NULL))
+		   NULL, 0))
 
 		/*
 		 * Any errors will already have been reported in the function process,
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index 9640838..0b7af54 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -36,6 +36,7 @@ static char *basedir = NULL;
 static int	verbose = 0;
 static int	noloop = 0;
 static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
+static int	fsync_interval = 0; /* 0 = default */
 static volatile bool time_to_abort = false;
 
 
@@ -62,6 +63,8 @@ usage(void)
 	printf(_(\nOptions:\n));
 	printf(_(  -D, --directory=DIRreceive transaction log files into this directory\n));
 	printf(_(  -n, --no-loop  do not loop on connection lost\n));
+	printf(_(  -F  --fsync-interval=INTERVAL\n
+			  frequency of syncs to transaction log files (in seconds)\n));
 	printf(_(  -v, --verbose  output verbose messages\n));
 	printf(_(  -V, --version  output version information, then exit\n));
 	printf(_(  -?, --help show this help, then exit\n));
@@ -330,7 +333,8 @@ StreamLog(void)
 starttli);
 
 	ReceiveXlogStream(conn, startpos, starttli, NULL, basedir,
-	  stop_streaming, standby_message_timeout, .partial);
+	  stop_streaming, standby_message_timeout, .partial,
+	  fsync_interval);
 
 	PQfinish(conn);
 }
@@ -360,6 +364,7 @@ main(int argc, char **argv)
 		{port, required_argument, NULL, 'p'},
 		{username, required_argument, NULL, 'U'},
 		{no-loop, no_argument, NULL, 'n'},
+		{fsync-interval, required_argument, NULL, 'F'},
 		{no-password, no_argument, NULL, 'w'},
 		{password, no_argument, NULL, 'W'},
 		{status-interval, required_argument, NULL, 's'},
@@ -389,7 +394,7 @@ main(int argc, char **argv)
 		}
 	}
 
-	while ((c = getopt_long(argc, argv, D:d:h:p:U:s:nwWv,
+	while ((c = getopt_long(argc, argv, D:d:h:p:U:s:nF:wWv,
 			long_options, option_index)) != -1)
 	{
 		switch (c)
@@ -436,6 +441,15 @@ main(int argc, char **argv)
 			case 'n':
 noloop = 1;
 break;
+		case 'F':
+			fsync_interval = atoi(optarg) * 1000;
+			if (fsync_interval  -1000)
+			{
+fprintf(stderr, _(%s: 

Re: [HACKERS] pg_receivexlog add synchronous mode

2014-08-05 Thread Fujii Masao
On Tue, Jul 29, 2014 at 7:07 PM,  furu...@pm.nttdata.co.jp wrote:
 I have improved the patch  by making following changes:

 1. Since stream_stop() was redundant, stream_stop() at the time of WAL file 
 closing was deleted.

 2. Change the Flash judging timing for the readability of source code.
I have changed the Flash judging timing , from the continuous message 
 after receiving to
before the feedbackmassege decision of continue statement after execution.

Thanks for the updated version of the patch!

While reviewing the patch, I found that HandleCopyStream() is still
long and which decreases the readability of the source code.
So I feel inclined to refactor the HandleCopyStream() more for better
readability. What about the attached refactoring patch?

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-08-05 Thread Fujii Masao
On Tue, Aug 5, 2014 at 9:04 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Tue, Jul 29, 2014 at 7:07 PM,  furu...@pm.nttdata.co.jp wrote:
 I have improved the patch  by making following changes:

 1. Since stream_stop() was redundant, stream_stop() at the time of WAL file 
 closing was deleted.

 2. Change the Flash judging timing for the readability of source code.
I have changed the Flash judging timing , from the continuous message 
 after receiving to
before the feedbackmassege decision of continue statement after execution.

 Thanks for the updated version of the patch!

 While reviewing the patch, I found that HandleCopyStream() is still
 long and which decreases the readability of the source code.
 So I feel inclined to refactor the HandleCopyStream() more for better
 readability. What about the attached refactoring patch?

Sorry, I forgot to attached the patch in previous email. So attached.

Regards,

-- 
Fujii Masao
*** a/src/bin/pg_basebackup/receivelog.c
--- b/src/bin/pg_basebackup/receivelog.c
***
*** 31,42  static char current_walfile_name[MAXPGPATH] = ;
--- 31,53 
  static bool reportFlushPosition = false;
  static XLogRecPtr lastFlushPosition = InvalidXLogRecPtr;
  
+ static bool still_sending = true;		/* feedback still needs to be sent? */
+ 
  static PGresult *HandleCopyStream(PGconn *conn, XLogRecPtr startpos,
   uint32 timeline, char *basedir,
  			   stream_stop_callback stream_stop, int standby_message_timeout,
   char *partial_suffix, XLogRecPtr *stoppos);
  static int CopyStreamPoll(PGconn *conn, long timeout_ms);
  static int CopyStreamReceive(PGconn *conn, long timeout, char **buffer);
+ static bool ProcessKeepaliveMsg(PGconn *conn, char *copybuf, int len,
+ XLogRecPtr blockpos, int64 *last_status);
+ static bool ProcessXLogDataMsg(PGconn *conn, char *copybuf, int len,
+ 			   XLogRecPtr *blockpos, uint32 timeline,
+ 			   char *basedir, stream_stop_callback stream_stop,
+ 			   char *partial_suffix);
+ static PGresult *HandleEndOfCopyStream(PGconn *conn, char *copybuf,
+ 	   XLogRecPtr blockpos, char *basedir, char *partial_suffix,
+ 	   XLogRecPtr *stoppos);
  
  static bool ReadEndOfStreamingResult(PGresult *res, XLogRecPtr *startpos,
  		 uint32 *timeline);
***
*** 740,755  HandleCopyStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline,
  	char	   *copybuf = NULL;
  	int64		last_status = -1;
  	XLogRecPtr	blockpos = startpos;
! 	bool		still_sending = true;
  
  	while (1)
  	{
  		int			r;
- 		int			xlogoff;
- 		int			bytes_left;
- 		int			bytes_written;
  		int64		now;
- 		int			hdr_len;
  		long		sleeptime;
  
  		/*
--- 751,763 
  	char	   *copybuf = NULL;
  	int64		last_status = -1;
  	XLogRecPtr	blockpos = startpos;
! 
! 	still_sending = true;
  
  	while (1)
  	{
  		int			r;
  		int64		now;
  		long		sleeptime;
  
  		/*
***
*** 818,1015  HandleCopyStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline,
  			goto error;
  		if (r == -2)
  		{
! 			PGresult   *res = PQgetResult(conn);
! 
! 			/*
! 			 * The server closed its end of the copy stream.  If we haven't
! 			 * closed ours already, we need to do so now, unless the server
! 			 * threw an error, in which case we don't.
! 			 */
! 			if (still_sending)
! 			{
! if (!close_walfile(basedir, partial_suffix, blockpos))
! {
! 	/* Error message written in close_walfile() */
! 	PQclear(res);
! 	goto error;
! }
! if (PQresultStatus(res) == PGRES_COPY_IN)
! {
! 	if (PQputCopyEnd(conn, NULL) = 0 || PQflush(conn))
! 	{
! 		fprintf(stderr,
! _(%s: could not send copy-end packet: %s),
! progname, PQerrorMessage(conn));
! 		PQclear(res);
! 		goto error;
! 	}
! 	PQclear(res);
! 	res = PQgetResult(conn);
! }
! still_sending = false;
! 			}
! 			if (copybuf != NULL)
! PQfreemem(copybuf);
! 			copybuf = NULL;
! 			*stoppos = blockpos;
! 			return res;
  		}
  
  		/* Check the message type. */
  		if (copybuf[0] == 'k')
  		{
! 			int			pos;
! 			bool		replyRequested;
! 
! 			/*
! 			 * Parse the keepalive message, enclosed in the CopyData message.
! 			 * We just check if the server requested a reply, and ignore the
! 			 * rest.
! 			 */
! 			pos = 1;			/* skip msgtype 'k' */
! 			pos += 8;			/* skip walEnd */
! 			pos += 8;			/* skip sendTime */
! 
! 			if (r  pos + 1)
! 			{
! fprintf(stderr, _(%s: streaming header too small: %d\n),
! 		progname, r);
  goto error;
- 			}
- 			replyRequested = copybuf[pos];
- 
- 			/* If the server requested an immediate reply, send one. */
- 			if (replyRequested  still_sending)
- 			{
- now = feGetCurrentTimestamp();
- if (!sendFeedback(conn, blockpos, now, false))
- 	goto error;
- last_status = now;
- 			}
  		}
  		else if (copybuf[0] == 'w')
  		{
! 			/*
! 			 * Once we've decided we don't want to receive any more, just
! 			 * ignore any subsequent 

Re: [HACKERS] pg_receivexlog add synchronous mode

2014-08-05 Thread furuyao
  I have improved the patch  by making following changes:
 
  1. Since stream_stop() was redundant, stream_stop() at the time of
 WAL file closing was deleted.
 
  2. Change the Flash judging timing for the readability of source code.
 I have changed the Flash judging timing , from the continuous
 message after receiving to
 before the feedbackmassege decision of continue statement after
 execution.
 
  Thanks for the updated version of the patch!
 
  While reviewing the patch, I found that HandleCopyStream() is still
  long and which decreases the readability of the source code.
  So I feel inclined to refactor the HandleCopyStream() more for better
  readability. What about the attached refactoring patch?
 
 Sorry, I forgot to attached the patch in previous email. So attached.

Thank you for the refactoring patch.
I did a review of the patch. 

-   break;  /* ignore the rest of 
this XLogData packet */

+   return true;/* ignore the rest of this 
XLogData packet */

For break statement at close of wal file, it is a return to true. 
It may be a behavior of continue statement. Is it satisfactory?

The walreceiver distributes XLogWalRcvProcessMsg and XLogWalRcvWrite, but isn't 
that division necessary?

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-07-29 Thread furuyao
I have improved the patch  by making following changes:

1. Since stream_stop() was redundant, stream_stop() at the time of WAL file 
closing was deleted. 

2. Change the Flash judging timing for the readability of source code.
   I have changed the Flash judging timing , from the continuous message after 
receiving to
   before the feedbackmassege decision of continue statement after execution.

Regards,

--
Furuya Osamu


pg_receivexlog-add-fsync-interval-v4.patch
Description: pg_receivexlog-add-fsync-interval-v4.patch

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-07-25 Thread furuyao
This patch was made by the following process.

1. post patch for additional pg_receivexlog synchronous mode.

2. In response to comment for the flush frequency, I revise the patch to do 
flush every consecutive message in reference to walreceiver.

3. The synchronization mode was necessary to reply after flush at a flush 
position, but --status-interval for setting at least 1 second, it was pointed 
out.
   Therefore I changed it to the patch to add the mode which the same interval 
flush with walreceiver, and I canceled the synchronization mode.

4. The refactor patch was offered, and there was the --fsync-interval option 
in the pg_recvlogical which specifies a flush interval.
   I changed it to the patch which did flush specified interval by the 
--fsync-interval in the same way as pg_recvlogical.

5. Post the patch that reflects the comments centered on the document pointed 
out that.

6. The patch for correcting the operation at the time of specifying zero as 
--status-interval was offered.
   In response to it, those that modify the behavior of If you specify -1 or 
zero of the fsync-interval is this patch.

Regards,

--
Furuya Osamu


pg_receivexlog-add-fsync-interval-v3.patch
Description: pg_receivexlog-add-fsync-interval-v3.patch

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-07-10 Thread furuyao
This patch is modified the comment.
Each comment is coping as follows.

 Could you update the status of this patch from Waiting on Author to
 Needs Review when you post the revised version of the patch?
Thank you for pointing out. 
From now on, I will update status when I post the patch.

 +How often should applicationpg_receivexlog/application
 issue sync
 +commands to ensure the received WAL file is safely
 +flushed to disk without being asked by the server to do so.
 
 without being asked by the server to do so implies that the server asks
 pg_receivexlog to flush WAL files periodically?
I think that the sentence means the following.
Without waiting for the feedback request from the server, select is timed out 
and flush is checked. 

Because I cause misunderstanding, I delete a sentence.

  Specifying
 +an interval of literal0/literal together the consecutive
 data.
 
 This text looks corrupted. What does this mean?
 
 +Not specifying an interval disables issuing fsyncs altogether,
 +while still reporting progress the server.
 
 No. Even when the option is not specified, WAL files should be flushed
 at WAL file switch, like current pg_receivexlog does. If you want to
 disable the flush completely, you can change the option so that it accepts
 -1 which means to disable the flush, for example.
Fix to description issuing fsyncs at only  WAL file close. 

 +printf(_(  -F  --fsync-interval=SECS\n
 +  frequency of syncs to the
 output file (default: file close only)\n));
 
 It's better to use transaction log files rather than output file
 here.
Fix as you pointed out.

 SECS should be INTERVAL for the sake of consistency with
 --stat-interval's help message?
Fix as you pointed out.

 + * fsync_interval controls how often we flush to the received
 + * WAL file, in seconds.
 
 seconds should be miliseconds?
Fix as you pointed out.

 The patch changes pg_receivexlog so that it keep processing the
 continuous messages without calling stream_stop(). But as I commented
 before, stream_stop should be called every time the message is received?
 Otherwise pg_basebackup background WAL receiver might not be able to stop
 streaming at proper point.
FIx the calling stream_stop() with 1 message processing is complete. 

 The flush interval is checked and WAL files are flushed only when
 CopyStreamReceive() returns 0, i.e., there is no message available and
 the timeout occurs. Why did you do that? I'm afraid that
 CopyStreamReceive() always waits for at least one second before WAL files
 are flushed even when --fsync-interval is set to 0.
CopyStreamReceive() is according to pg_recvlogical --fsync-interval and 
--status-interval shorter intervals runs the wait.
About specifying an interval of zero.
Every flush to continuously message,  so no problem will wait one second.

 Why don't you update output_last_status when WAL file is flushed and
 closed?
About the closed, add the update step.
About the flush, according to pg_recvlogical, update is performed after an 
interval check before flush. 
Therefore not modify.

Regards,

--
Furuya Osamu



pg_receivexlog-add-fsync-interval-v2.patch
Description: pg_receivexlog-add-fsync-interval-v2.patch

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-07-07 Thread Fujii Masao
On Fri, Jul 4, 2014 at 7:45 PM,  furu...@pm.nttdata.co.jp wrote:
  Thanks for reviewing the patch!
 
  I think that this refactoring patch is useful for improving source
  code readability and making the future patches simpler, whether we
  adopt your patch or not. So, barring any objections, I'm thinking to
  commit this refactoring patch.

 Committed!
 It is a patch that added the --fsync-interval option.
 Interface of --fsync-interval option was referring to the pg_recvlogical.c.

 It is not judgement the flush on a per-message basis.
 It is judgment at the time of receipt stop of the message.

 If you specify a zero --fsync-interval make the flush at the same timing as 
 the walreceiver .
 If you do not specify --fsync-interval, you will flush only when switching as 
 WAL files as in the past.

Thanks for revising the patch!

Could you update the status of this patch from Waiting on Author to
Needs Review when you post the revised version of the patch?

+How often should applicationpg_receivexlog/application issue sync
+commands to ensure the received WAL file is safely
+flushed to disk without being asked by the server to do so.

without being asked by the server to do so implies that the server asks
pg_receivexlog to flush WAL files periodically?

 Specifying
+an interval of literal0/literal together the consecutive data.

This text looks corrupted. What does this mean?

+Not specifying an interval disables issuing fsyncs altogether,
+while still reporting progress the server.

No. Even when the option is not specified, WAL files should be flushed at
WAL file switch, like current pg_receivexlog does. If you want to disable
the flush completely, you can change the option so that it accepts -1 which
means to disable the flush, for example.

+printf(_(  -F  --fsync-interval=SECS\n
+  frequency of syncs to the
output file (default: file close only)\n));

It's better to use transaction log files rather than output file here.

SECS should be INTERVAL for the sake of consistency with --stat-interval's
help message?

+ * fsync_interval controls how often we flush to the received
+ * WAL file, in seconds.

seconds should be miliseconds?

The patch changes pg_receivexlog so that it keep processing
the continuous messages without calling stream_stop(). But
as I commented before, stream_stop should be called every time
the message is received? Otherwise pg_basebackup background
WAL receiver might not be able to stop streaming at proper point.

The flush interval is checked and WAL files are flushed only when
CopyStreamReceive() returns 0, i.e., there is no message available
and the timeout occurs. Why did you do that? I'm afraid that
CopyStreamReceive() always waits for at least one second before
WAL files are flushed even when --fsync-interval is set to 0.

Why don't you update output_last_status when WAL file is flushed
and closed?

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-07-04 Thread furuyao
  Thanks for reviewing the patch!
 
  I think that this refactoring patch is useful for improving source
  code readability and making the future patches simpler, whether we
  adopt your patch or not. So, barring any objections, I'm thinking to
  commit this refactoring patch.
 
 Committed!
It is a patch that added the --fsync-interval option.
Interface of --fsync-interval option was referring to the pg_recvlogical.c.

It is not judgement the flush on a per-message basis. 
It is judgment at the time of receipt stop of the message.

If you specify a zero --fsync-interval make the flush at the same timing as the 
walreceiver .
If you do not specify --fsync-interval, you will flush only when switching as 
WAL files as in the past.

Regards,

--
Furuya Osamu



pg_receivexlog-add-fsync-interval-v1.patch
Description: pg_receivexlog-add-fsync-interval-v1.patch

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-07-03 Thread Fujii Masao
On Tue, Jul 1, 2014 at 10:11 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Mon, Jun 30, 2014 at 7:09 PM,  furu...@pm.nttdata.co.jp wrote:
 Thanks for the review!

 +if (secs = 0)
 +secs = 1;/* Always sleep at least 1 sec */
 +
 +sleeptime = secs * 1000 + usecs / 1000;

 The above is the code which caused that problem. 'usecs' should have been
 reset to zero when 'secs' are rounded up to 1 second. But not. Attached
 is the updated version of the patch.
 Thank you for the refactoring v2 patch.
 I did a review of the patch.

 1. applied cleanly and compilation was without warnings and errors
 2. all regress tests was passed ok
 3. sleeptime is ok when the --status-intarvall is set to 1

 Thanks for reviewing the patch!

 I think that this refactoring patch is useful for improving source code
 readability and making the future patches simpler, whether we adopt
 your patch or not. So, barring any objections, I'm thinking to commit
 this refactoring patch.

Committed!

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-07-01 Thread Fujii Masao
On Mon, Jun 30, 2014 at 7:09 PM,  furu...@pm.nttdata.co.jp wrote:
 Thanks for the review!

 +if (secs = 0)
 +secs = 1;/* Always sleep at least 1 sec */
 +
 +sleeptime = secs * 1000 + usecs / 1000;

 The above is the code which caused that problem. 'usecs' should have been
 reset to zero when 'secs' are rounded up to 1 second. But not. Attached
 is the updated version of the patch.
 Thank you for the refactoring v2 patch.
 I did a review of the patch.

 1. applied cleanly and compilation was without warnings and errors
 2. all regress tests was passed ok
 3. sleeptime is ok when the --status-intarvall is set to 1

Thanks for reviewing the patch!

I think that this refactoring patch is useful for improving source code
readability and making the future patches simpler, whether we adopt
your patch or not. So, barring any objections, I'm thinking to commit
this refactoring patch.

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-30 Thread furuyao
 Thanks for the review!
 
 +if (secs = 0)
 +secs = 1;/* Always sleep at least 1 sec */
 +
 +sleeptime = secs * 1000 + usecs / 1000;
 
 The above is the code which caused that problem. 'usecs' should have been
 reset to zero when 'secs' are rounded up to 1 second. But not. Attached
 is the updated version of the patch.
Thank you for the refactoring v2 patch.
I did a review of the patch. 

1. applied cleanly and compilation was without warnings and errors
2. all regress tests was passed ok
3. sleeptime is ok when the --status-intarvall is set to 1

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-30 Thread furuyao
Synchronous(synchronous_commit = on) mode offers the ability to
   confirm WAL have been streamed in the same way as synchronous
   replication.
If an output is used as a different disk from the directory where
the
   transaction log should be stored.
Prevent the loss of data due to disk failure.
   
the additional parameter(-m) and replicationslot specify, that its
   synchronous mode.
All received WAL write after, flush is executed and reply flush
position.
  
   What's the usecase for this? I can see some benefit in easier
   testing of syncrep, but that's basically it?
 
  When used with syncrep, standby server crashes, multiplexing of WAL
 can be collateral.
  Data loss can be to nearly zero.
 
 I don't see how this gets pg_receivexlog much closer to a solution for
 multiplexing WAL. Since there's no support for streaming data, removing
 old WAL and such it seems to me you'd need to have something entirely
 different anyway?
 I'm not really averse to adding this feature (on the basis of easier
 syncrep testing alone), but I don't like the arguments for it so far...

The problems of multiplex WAL which I recognize as follows.

1.To flush multiple records per received consecutively. (implemented in 
pg_receivexlog)
2.A feedback message reports a flush position for every flush. (implemented in 
pg_receivexlog)
3.establishment of recovery steps by using pg_receivexlog.
4.removing old WAL.(Remove the recycled or archived WAL)

First, it is not considered multiple WAL. 
I will post a patch to flush to multiple records for each received continuously.

By increasing the frequency of flush, 
this patch reduces the lost of data of pg_receivexlog machine crash.

I will consider in turn also other problems.

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-27 Thread Fujii Masao
On Thu, Jun 26, 2014 at 7:01 PM,  furu...@pm.nttdata.co.jp wrote:
 The patch looks somewhat complicated and bugs can be easily introduced
 because it tries to not only add new feature but also reorganize the main
 loop in HandleCopyStream at the same time. To keep the patch simple, I'm
 thinking to firstly apply the attached patch which just refactors the
 main loop. Then we can apply the main patch, i.e., add new feature.
 Thought?
 Thank you for the refactoring patch.
 I did a review of the patch.
 As a result, I found the calculation of sleeptime when the --status-intarvall 
 is set to 1 was incorrect.

 --status-intarvall 1 --  sleeptime 1. !?
 --status-intarvall 2 --  sleeptime 1. OK
 --status-intarvall 3 --  sleeptime 2. OK

Thanks for the review!

+if (secs = 0)
+secs = 1;/* Always sleep at least 1 sec */
+
+sleeptime = secs * 1000 + usecs / 1000;

The above is the code which caused that problem. 'usecs' should have been
reset to zero when 'secs' are rounded up to 1 second. But not. Attached is
the updated version of the patch.

Regards,

-- 
Fujii Masao
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index d76e605..4aa35da 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -35,6 +35,8 @@ static PGresult *HandleCopyStream(PGconn *conn, XLogRecPtr startpos,
  uint32 timeline, char *basedir,
 			   stream_stop_callback stream_stop, int standby_message_timeout,
  char *partial_suffix, XLogRecPtr *stoppos);
+static int CopyStreamPoll(PGconn *conn, long timeout_ms);
+static int CopyStreamReceive(PGconn *conn, long timeout, char **buffer);
 
 static bool ReadEndOfStreamingResult(PGresult *res, XLogRecPtr *startpos,
 		 uint32 *timeline);
@@ -744,12 +746,7 @@ HandleCopyStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline,
 		int			bytes_written;
 		int64		now;
 		int			hdr_len;
-
-		if (copybuf != NULL)
-		{
-			PQfreemem(copybuf);
-			copybuf = NULL;
-		}
+		long		sleeptime;
 
 		/*
 		 * Check if we should continue streaming, or abort at this point.
@@ -784,67 +781,38 @@ HandleCopyStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline,
 			last_status = now;
 		}
 
-		r = PQgetCopyData(conn, copybuf, 1);
-		if (r == 0)
+		/*
+		 * Compute how long send/receive loops should sleep
+		 */
+		if (standby_message_timeout  still_sending)
 		{
-			/*
-			 * No data available. Wait for some to appear, but not longer than
-			 * the specified timeout, so that we can ping the server.
-			 */
-			fd_set		input_mask;
-			struct timeval timeout;
-			struct timeval *timeoutptr;
-
-			FD_ZERO(input_mask);
-			FD_SET(PQsocket(conn), input_mask);
-			if (standby_message_timeout  still_sending)
+			int64		targettime;
+			long		secs;
+			int			usecs;
+
+			targettime = last_status + (standby_message_timeout - 1) * ((int64) 1000);
+			feTimestampDifference(now,
+  targettime,
+  secs,
+  usecs);
+			/* Always sleep at least 1 sec */
+			if (secs = 0)
 			{
-int64		targettime;
-long		secs;
-int			usecs;
-
-targettime = last_status + (standby_message_timeout - 1) * ((int64) 1000);
-feTimestampDifference(now,
-	  targettime,
-	  secs,
-	  usecs);
-if (secs = 0)
-	timeout.tv_sec = 1; /* Always sleep at least 1 sec */
-else
-	timeout.tv_sec = secs;
-timeout.tv_usec = usecs;
-timeoutptr = timeout;
+secs = 1;
+usecs = 0;
 			}
-			else
-timeoutptr = NULL;
 
-			r = select(PQsocket(conn) + 1, input_mask, NULL, NULL, timeoutptr);
-			if (r == 0 || (r  0  errno == EINTR))
-			{
-/*
- * Got a timeout or signal. Continue the loop and either
- * deliver a status packet to the server or just go back into
- * blocking.
- */
-continue;
-			}
-			else if (r  0)
-			{
-fprintf(stderr, _(%s: select() failed: %s\n),
-		progname, strerror(errno));
-goto error;
-			}
-			/* Else there is actually data on the socket */
-			if (PQconsumeInput(conn) == 0)
-			{
-fprintf(stderr,
-		_(%s: could not receive data from WAL stream: %s),
-		progname, PQerrorMessage(conn));
-goto error;
-			}
-			continue;
+			sleeptime = secs * 1000 + usecs / 1000;
 		}
+		else
+			sleeptime = -1;
+
+		r = CopyStreamReceive(conn, sleeptime, copybuf);
+		if (r == 0)
+			continue;
 		if (r == -1)
+			goto error;
+		if (r == -2)
 		{
 			PGresult   *res = PQgetResult(conn);
 
@@ -877,15 +845,10 @@ HandleCopyStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline,
 			}
 			if (copybuf != NULL)
 PQfreemem(copybuf);
+			copybuf = NULL;
 			*stoppos = blockpos;
 			return res;
 		}
-		if (r == -2)
-		{
-			fprintf(stderr, _(%s: could not read COPY data: %s),
-	progname, PQerrorMessage(conn));
-			goto error;
-		}
 
 		/* Check the message type. */
 		if (copybuf[0] == 'k')
@@ -1056,3 +1019,115 @@ error:
 		PQfreemem(copybuf);
 	return NULL;
 }
+
+/*
+ * Wait 

Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-27 Thread Andres Freund
On 2014-06-05 19:13:34 +0900, furu...@pm.nttdata.co.jp wrote:
  -Original Message-
  Hi,
  
  On 2014-06-05 17:09:44 +0900, furu...@pm.nttdata.co.jp wrote:
   Synchronous(synchronous_commit = on) mode offers the ability to
  confirm WAL have been streamed in the same way as synchronous
  replication.
   If an output is used as a different disk from the directory where the
  transaction log should be stored.
   Prevent the loss of data due to disk failure.
  
   the additional parameter(-m) and replicationslot specify, that its
  synchronous mode.
   All received WAL write after, flush is executed and reply flush
   position.
  
  What's the usecase for this? I can see some benefit in easier testing
  of syncrep, but that's basically it?
 
 When used with syncrep, standby server crashes, multiplexing of WAL can be 
 collateral.
 Data loss can be to nearly zero.

I don't see how this gets pg_receivexlog much closer to a solution for
multiplexing WAL. Since there's no support for streaming data, removing
old WAL and such it seems to me you'd need to have something entirely
different anyway?
I'm not really averse to adding this feature (on the basis of easier
syncrep testing alone), but I don't like the arguments for it so far...

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-26 Thread furuyao
 The patch looks somewhat complicated and bugs can be easily introduced
 because it tries to not only add new feature but also reorganize the main
 loop in HandleCopyStream at the same time. To keep the patch simple, I'm
 thinking to firstly apply the attached patch which just refactors the
 main loop. Then we can apply the main patch, i.e., add new feature.
 Thought?
Thank you for the refactoring patch.
I did a review of the patch.
As a result, I found the calculation of sleeptime when the --status-intarvall 
is set to 1 was incorrect.

--status-intarvall 1 --  sleeptime 1. !?
--status-intarvall 2 --  sleeptime 1. OK
--status-intarvall 3 --  sleeptime 2. OK

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-25 Thread Fujii Masao
On Wed, Jun 25, 2014 at 3:50 AM, Fujii Masao masao.fu...@gmail.com wrote:
 On Tue, Jun 24, 2014 at 3:18 PM,  furu...@pm.nttdata.co.jp wrote:
 I found that this patch breaks --status-interval option of
 pg_receivexlog when -m option which the patch introduced is supplied.
 When -m is set, pg_receivexlog tries to send the feedback message as soon
 as it flushes WAL file even if status interval timeout has not been passed
 yet. If you want to send the feedback as soon as WAL is written or flushed,
 like walreceiver does, you need to extend --status-interval option, for
 example, so that it accepts the value -1 which means enabling that
 behavior.

 Including this change in your original patch would make it more difficult
 to review. I think that you should implement this as separate patch.
 Thought?
 As your comments, the current specification to ignore the --status-intarvall.
 It is necessary to respond immediately to synchronize.

 It is necessary to think about specifications the --status-intarvall.
 So I revised it to a patch of flushmode which performed flush by a timing 
 same as walreceiver.

 I'm not sure if it's good idea to call the feature which you'd like to
 add as 'flush mode'.
 ISTM that 'flush mode' is vague and confusion for users. Instead, what
 about adding
 something like --fsync-interval which pg_recvlogical supports?

 A changed part deletes the feedback message after flush, and transmitted the 
 feedback message according to the status interval.
 Change to flushmode from syncmode the mode name, and fixed the document.

 + * Receive a message available from XLOG stream, blocking for
 + * maximum of 'timeout' ms.

 The above comment seems incorrect because 'timeout' is boolean argument.

 +FD_ZERO(input_mask);
 +FD_SET(PQsocket(conn), input_mask);
 +if (standby_message_timeout)

 Why did you get rid of the check of 'still_sending' flag here? Originally the
 flag was checked but not in the patch.

 +r = rcv_receive(true , copybuf, conn,
 standby_message_timeout, last_status, now);

 When the return value is -2 (i.e., an error happend), we should go to
 the 'error' label.

 ISTM that stream_stop() should be called every time a message is
 processed. But the
 patch changes pg_receivexlog so that it keeps processing the received
 data without
 calling stream_stop(). This seems incorrect.

 'copybuf' needs to be free'd every time new message is received. But you seem 
 to
 have forgotten to do that when rcv_receive() with no timeout is called.

The patch looks somewhat complicated and bugs can be easily introduced
because it tries to not only add new feature but also reorganize
the main loop in HandleCopyStream at the same time. To keep the patch
simple, I'm thinking to firstly apply the attached patch which just
refactors the main loop. Then we can apply the main patch, i.e., add new
feature. Thought?

Regards,

-- 
Fujii Masao
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index d76e605..1182dc7 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -35,6 +35,8 @@ static PGresult *HandleCopyStream(PGconn *conn, XLogRecPtr startpos,
  uint32 timeline, char *basedir,
 			   stream_stop_callback stream_stop, int standby_message_timeout,
  char *partial_suffix, XLogRecPtr *stoppos);
+static int CopyStreamPoll(PGconn *conn, long timeout_ms);
+static int CopyStreamReceive(PGconn *conn, long timeout, char **buffer);
 
 static bool ReadEndOfStreamingResult(PGresult *res, XLogRecPtr *startpos,
 		 uint32 *timeline);
@@ -744,12 +746,7 @@ HandleCopyStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline,
 		int			bytes_written;
 		int64		now;
 		int			hdr_len;
-
-		if (copybuf != NULL)
-		{
-			PQfreemem(copybuf);
-			copybuf = NULL;
-		}
+		long		sleeptime;
 
 		/*
 		 * Check if we should continue streaming, or abort at this point.
@@ -784,67 +781,34 @@ HandleCopyStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline,
 			last_status = now;
 		}
 
-		r = PQgetCopyData(conn, copybuf, 1);
-		if (r == 0)
+		/*
+		 * Compute how long send/receive loops should sleep
+		 */
+		if (standby_message_timeout  still_sending)
 		{
-			/*
-			 * No data available. Wait for some to appear, but not longer than
-			 * the specified timeout, so that we can ping the server.
-			 */
-			fd_set		input_mask;
-			struct timeval timeout;
-			struct timeval *timeoutptr;
-
-			FD_ZERO(input_mask);
-			FD_SET(PQsocket(conn), input_mask);
-			if (standby_message_timeout  still_sending)
-			{
-int64		targettime;
-long		secs;
-int			usecs;
-
-targettime = last_status + (standby_message_timeout - 1) * ((int64) 1000);
-feTimestampDifference(now,
-	  targettime,
-	  secs,
-	  usecs);
-if (secs = 0)
-	timeout.tv_sec = 1; /* Always sleep at least 1 sec */
-else
-	timeout.tv_sec = secs;
-timeout.tv_usec = usecs;
-timeoutptr = 

Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-24 Thread furuyao
 I found that this patch breaks --status-interval option of
 pg_receivexlog when -m option which the patch introduced is supplied.
 When -m is set, pg_receivexlog tries to send the feedback message as soon
 as it flushes WAL file even if status interval timeout has not been passed
 yet. If you want to send the feedback as soon as WAL is written or flushed,
 like walreceiver does, you need to extend --status-interval option, for
 example, so that it accepts the value -1 which means enabling that
 behavior.
 
 Including this change in your original patch would make it more difficult
 to review. I think that you should implement this as separate patch.
 Thought?
As your comments, the current specification to ignore the --status-intarvall.
It is necessary to respond immediately to synchronize.

It is necessary to think about specifications the --status-intarvall.
So I revised it to a patch of flushmode which performed flush by a timing same 
as walreceiver.

A changed part deletes the feedback message after flush, and transmitted the 
feedback message according to the status interval. 
Change to flushmode from syncmode the mode name, and fixed the document.

Regards,

-- 
Furuya Osamu


pg_receivexlog-add-flush-mode-v1.patch
Description: pg_receivexlog-add-flush-mode-v1.patch

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-24 Thread Fujii Masao
On Tue, Jun 24, 2014 at 3:18 PM,  furu...@pm.nttdata.co.jp wrote:
 I found that this patch breaks --status-interval option of
 pg_receivexlog when -m option which the patch introduced is supplied.
 When -m is set, pg_receivexlog tries to send the feedback message as soon
 as it flushes WAL file even if status interval timeout has not been passed
 yet. If you want to send the feedback as soon as WAL is written or flushed,
 like walreceiver does, you need to extend --status-interval option, for
 example, so that it accepts the value -1 which means enabling that
 behavior.

 Including this change in your original patch would make it more difficult
 to review. I think that you should implement this as separate patch.
 Thought?
 As your comments, the current specification to ignore the --status-intarvall.
 It is necessary to respond immediately to synchronize.

 It is necessary to think about specifications the --status-intarvall.
 So I revised it to a patch of flushmode which performed flush by a timing 
 same as walreceiver.

I'm not sure if it's good idea to call the feature which you'd like to
add as 'flush mode'.
ISTM that 'flush mode' is vague and confusion for users. Instead, what
about adding
something like --fsync-interval which pg_recvlogical supports?

 A changed part deletes the feedback message after flush, and transmitted the 
 feedback message according to the status interval.
 Change to flushmode from syncmode the mode name, and fixed the document.

+ * Receive a message available from XLOG stream, blocking for
+ * maximum of 'timeout' ms.

The above comment seems incorrect because 'timeout' is boolean argument.

+FD_ZERO(input_mask);
+FD_SET(PQsocket(conn), input_mask);
+if (standby_message_timeout)

Why did you get rid of the check of 'still_sending' flag here? Originally the
flag was checked but not in the patch.

+r = rcv_receive(true , copybuf, conn,
standby_message_timeout, last_status, now);

When the return value is -2 (i.e., an error happend), we should go to
the 'error' label.

ISTM that stream_stop() should be called every time a message is
processed. But the
patch changes pg_receivexlog so that it keeps processing the received
data without
calling stream_stop(). This seems incorrect.

'copybuf' needs to be free'd every time new message is received. But you seem to
have forgotten to do that when rcv_receive() with no timeout is called.

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-19 Thread Fujii Masao
On Mon, Jun 16, 2014 at 7:03 PM,  furu...@pm.nttdata.co.jp wrote:
 You introduced the state machine using the flag flush_flg into
 pg_receivexlog.
 That's complicated and would reduce the readability of the source code.
 I think that the logic should be simpler like walreceiver's one.

 Maybe I found one problematic path as follows:

 1. WAL is written and flush_flag is set to 1 2. PQgetCopyData() returns
 0 and flush_flg is incremented to 2 3. PQconsumeInput() is executed 4.
 PQgetCopyData() reads keepalive message 5. After processing keepalive
 message, PQgetCopyDate() returns 0 6. Since flush_flg is 2, WAL is
 flushed and flush_flg is reset to 0

 But new message can arrive while processing keepalive message. Before
 flushing WAL, such new message should be processed.
 Together with the readability, fixed to the same process as the loop of 
 walreceiver.

 +Enables synchronous mode. If replication slot is disabled then
 +this setting is irrelevant.

 Why is that irrelevant in that case?

 Even when replication slot is not used, thanks to this feature,
 pg_receivexlog can flush WAL more proactively and which may improve the
 durability of WAL which pg_receivexlog writes.
 It's mean, report the flush position or not.
 If the SLOT is not used, it is not reported.
 Fixed to be reported only when using the SLOT.

 +printf(_(  -m, --sync-modesynchronous mode\n));

 I think that calling this feature synchronous mode is confusing.
 Modified the synchronous mode to this mode is written some records, flush 
 them to disk..

I found that this patch breaks --status-interval option of pg_receivexlog
when -m option which the patch introduced is supplied. When -m is set,
pg_receivexlog tries to send the feedback message as soon as it flushes
WAL file even if status interval timeout has not been passed yet. If you
want to send the feedback as soon as WAL is written or flushed, like
walreceiver does, you need to extend --status-interval option, for example,
so that it accepts the value -1 which means enabling that behavior.

Including this change in your original patch would make it more difficult
to review. I think that you should implement this as separate patch.
Thought?

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-16 Thread furuyao
 You introduced the state machine using the flag flush_flg into
 pg_receivexlog.
 That's complicated and would reduce the readability of the source code.
 I think that the logic should be simpler like walreceiver's one.
 
 Maybe I found one problematic path as follows:
 
 1. WAL is written and flush_flag is set to 1 2. PQgetCopyData() returns
 0 and flush_flg is incremented to 2 3. PQconsumeInput() is executed 4.
 PQgetCopyData() reads keepalive message 5. After processing keepalive
 message, PQgetCopyDate() returns 0 6. Since flush_flg is 2, WAL is
 flushed and flush_flg is reset to 0
 
 But new message can arrive while processing keepalive message. Before
 flushing WAL, such new message should be processed.
Together with the readability, fixed to the same process as the loop of 
walreceiver.

 +Enables synchronous mode. If replication slot is disabled then
 +this setting is irrelevant.
 
 Why is that irrelevant in that case?
 
 Even when replication slot is not used, thanks to this feature,
 pg_receivexlog can flush WAL more proactively and which may improve the
 durability of WAL which pg_receivexlog writes.
It's mean, report the flush position or not.
If the SLOT is not used, it is not reported.
Fixed to be reported only when using the SLOT.

 +printf(_(  -m, --sync-modesynchronous mode\n));
 
 I think that calling this feature synchronous mode is confusing.
Modified the synchronous mode to this mode is written some records, flush 
them to disk..

Regards,

-- 
Furuya Osamu


pg_receivexlog-add-synchronous-mode-v4.patch
Description: pg_receivexlog-add-synchronous-mode-v4.patch

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-11 Thread Fujii Masao
On Tue, Jun 10, 2014 at 5:01 PM,  furu...@pm.nttdata.co.jp wrote:
 No. IIUC walreceiver does flush *less* frequently than what you
 implemented on pg_receivexlog. Your version of pg_receivexlog tries to
 do flush every time when it receives one WAL chunk. OTOH, walreceiver
 does flush only when there is no extra WAL chunk in receive buffer. IOW,
 after writing WAL chunk, if there is another WAL chunk that walreceiver
 can receive immediately, it postpones flush later.

  However, it seems difficult to apply as same way.

 Why? ISTM that's not so difficult.

 I was not able to understand movement of walreceiver well.
 While walreceiver writes data, do PQconsumeInput() by omitting the select().
 Do flush if the PQgetCopyData has been to return the zero continuously.
 Fixed to the same process using the flag.

You introduced the state machine using the flag flush_flg into pg_receivexlog.
That's complicated and would reduce the readability of the source code. I think
that the logic should be simpler like walreceiver's one.

Maybe I found one problematic path as follows:

1. WAL is written and flush_flag is set to 1
2. PQgetCopyData() returns 0 and flush_flg is incremented to 2
3. PQconsumeInput() is executed
4. PQgetCopyData() reads keepalive message
5. After processing keepalive message, PQgetCopyDate() returns 0
6. Since flush_flg is 2, WAL is flushed and flush_flg is reset to 0

But new message can arrive while processing keepalive message. Before
flushing WAL, such new message should be processed.

+Enables synchronous mode. If replication slot is disabled then
+this setting is irrelevant.

Why is that irrelevant in that case?

Even when replication slot is not used, thanks to this feature, pg_receivexlog
can flush WAL more proactively and which may improve the durability of WAL
which pg_receivexlog writes.

+printf(_(  -m, --sync-modesynchronous mode\n));

I think that calling this feature synchronous mode is confusing.

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-10 Thread furuyao
 No. IIUC walreceiver does flush *less* frequently than what you
 implemented on pg_receivexlog. Your version of pg_receivexlog tries to
 do flush every time when it receives one WAL chunk. OTOH, walreceiver
 does flush only when there is no extra WAL chunk in receive buffer. IOW,
 after writing WAL chunk, if there is another WAL chunk that walreceiver
 can receive immediately, it postpones flush later.
 
  However, it seems difficult to apply as same way.
 
 Why? ISTM that's not so difficult.

I was not able to understand movement of walreceiver well.
While walreceiver writes data, do PQconsumeInput() by omitting the select().
Do flush if the PQgetCopyData has been to return the zero continuously.
Fixed to the same process using the flag.

Regards,

-- 
Furuya Osamu



pg_receivexlog-add-synchronous-mode-v3.patch
Description: pg_receivexlog-add-synchronous-mode-v3.patch

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-06 Thread furuyao
 -Original Message-
   Flush is not performed every time write, it is performed
   collectively like walrecever.
 
  I only glanced at this, but afaics you're only flushing at the end
  every WAL segment. That will result in absolutely horrible performance,
 right?
  Walreceiver does flush more frequently than that. It basically syncs
  every chunk of received WAL...
 
 IMO the completion of the write loop was completion of received WAL.
 And Walreceiver same.
 
 I confirm it about the flush position.
As you say,Walreceiver does flush more frequently than that.
However, it seems difficult to apply as same way.
So, I have tried a different approach.

1. select ()  time-out 100msec setting.
2. flush check is time-out of the select ().
3. wirte() only when flush.

I think this is what cause the problem, but I don't have some good idea to 
solve it.
Can someone please advise me?

Regards,

-- 
Furuya Osamu


pg_receivexlog-add-synchronous-mode-v2.patch
Description: pg_receivexlog-add-synchronous-mode-v2.patch

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-06 Thread Fujii Masao
On Fri, Jun 6, 2014 at 8:05 PM,  furu...@pm.nttdata.co.jp wrote:
 -Original Message-
   Flush is not performed every time write, it is performed
   collectively like walrecever.
 
  I only glanced at this, but afaics you're only flushing at the end
  every WAL segment. That will result in absolutely horrible performance,
 right?
  Walreceiver does flush more frequently than that. It basically syncs
  every chunk of received WAL...

 IMO the completion of the write loop was completion of received WAL.
 And Walreceiver same.

 I confirm it about the flush position.
 As you say,Walreceiver does flush more frequently than that.

No. IIUC walreceiver does flush *less* frequently than what you implemented
on pg_receivexlog. Your version of pg_receivexlog tries to do flush every time
when it receives one WAL chunk. OTOH, walreceiver does flush only when
there is no extra WAL chunk in receive buffer. IOW, after writing WAL chunk,
if there is another WAL chunk that walreceiver can receive immediately, it
postpones flush later.

 However, it seems difficult to apply as same way.

Why? ISTM that's not so difficult.

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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-05 Thread Andres Freund
Hi,

On 2014-06-05 17:09:44 +0900, furu...@pm.nttdata.co.jp wrote:
 Synchronous(synchronous_commit = on) mode offers the ability to confirm WAL 
 have been streamed in the same way as synchronous replication.
 If an output is used as a different disk from the directory where the 
 transaction log should be stored.
 Prevent the loss of data due to disk failure.
 
 the additional parameter(-m) and replicationslot specify, that its 
 synchronous mode.
 All received WAL write after, flush is executed and reply flush
 position.

What's the usecase for this? I can see some benefit in easier testing of
syncrep, but that's basically it?

 Flush is not performed every time write, it is performed collectively
 like walrecever. 

I only glanced at this, but afaics you're only flushing at the end every
WAL segment. That will result in absolutely horrible performance, right?
Walreceiver does flush more frequently than that. It basically syncs
every chunk of received WAL...

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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


Re: [HACKERS] pg_receivexlog add synchronous mode

2014-06-05 Thread furuyao
 -Original Message-
 Hi,
 
 On 2014-06-05 17:09:44 +0900, furu...@pm.nttdata.co.jp wrote:
  Synchronous(synchronous_commit = on) mode offers the ability to
 confirm WAL have been streamed in the same way as synchronous
 replication.
  If an output is used as a different disk from the directory where the
 transaction log should be stored.
  Prevent the loss of data due to disk failure.
 
  the additional parameter(-m) and replicationslot specify, that its
 synchronous mode.
  All received WAL write after, flush is executed and reply flush
  position.
 
 What's the usecase for this? I can see some benefit in easier testing
 of syncrep, but that's basically it?

When used with syncrep, standby server crashes, multiplexing of WAL can be 
collateral.
Data loss can be to nearly zero.

  Flush is not performed every time write, it is performed collectively
  like walrecever.
 
 I only glanced at this, but afaics you're only flushing at the end every
 WAL segment. That will result in absolutely horrible performance, right?
 Walreceiver does flush more frequently than that. It basically syncs
 every chunk of received WAL...

IMO the completion of the write loop was completion of received WAL.
And Walreceiver same.

I confirm it about the flush position.

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