On Mon, Jun 16, 2014 at 7:03 PM, <furuyao@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-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
