Re: pg_receivexlog add synchronous mode

Hi,

This patch implements a pg_receivexlog add synchronous mode.
Now, synchronous(synchronous_commit = remote_write) is supported.
But synchronous(synchronous_commit = remote_write), if the server crashes then WAL file may not to be flushed to disk , causing data loss.

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.
Flush is not performed every time write, it is performed collectively like walrecever.

Regards,

--
Furuya Osamu

Hi,

On 2014-06-05 17:09:44 +0900, furuyao(at)pm(dot)nttdata(dot)co(dot)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

> -----Original Message-----
> Hi,
>
> On 2014-06-05 17:09:44 +0900, furuyao(at)pm(dot)nttdata(dot)co(dot)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

> -----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

On Fri, Jun 6, 2014 at 8:05 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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

> 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

On Tue, Jun 10, 2014 at 5:01 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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-mode synchronous mode\n"));

I think that calling this feature "synchronous mode" is confusing.

Regards,

--
Fujii Masao

> 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-mode synchronous 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

On Mon, Jun 16, 2014 at 7:03 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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-mode synchronous 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

> 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

On Tue, Jun 24, 2014 at 3:18 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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

On Wed, Jun 25, 2014 at 3:50 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Jun 24, 2014 at 3:18 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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

> 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.9999 !?
--status-intarvall 2 --> sleeptime 1.9999 OK
--status-intarvall 3 --> sleeptime 2.9999 OK

Regards,

--
Furuya Osamu

On Thu, Jun 26, 2014 at 7:01 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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.9999 !?
> --status-intarvall 2 --> sleeptime 1.9999 OK
> --status-intarvall 3 --> sleeptime 2.9999 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

On 2014-06-05 19:13:34 +0900, furuyao(at)pm(dot)nttdata(dot)co(dot)jp wrote:
> > -----Original Message-----
> > Hi,
> >
> > On 2014-06-05 17:09:44 +0900, furuyao(at)pm(dot)nttdata(dot)co(dot)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

> 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

> > > > 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

On Mon, Jun 30, 2014 at 7:09 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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

On Tue, Jul 1, 2014 at 10:11 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Mon, Jun 30, 2014 at 7:09 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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

> > 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

On Fri, Jul 4, 2014 at 7:45 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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 <application>pg_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 <literal>0</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

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 <application>pg_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 <literal>0</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

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

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

On Tue, Jul 29, 2014 at 7:07 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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

On Tue, Aug 5, 2014 at 9:04 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Jul 29, 2014 at 7:07 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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

> >> 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

On Wed, Aug 6, 2014 at 2:34 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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

> > - 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

On Wed, Aug 6, 2014 at 5:10 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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

> 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

On Thu, Aug 7, 2014 at 5:24 PM, <furuyao(at)pm(dot)nttdata(dot)co(dot)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

On Fri, Aug 8, 2014 at 5:10 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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

On Tue, Aug 12, 2014 at 4:34 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Aug 8, 2014 at 5:10 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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