On Thu, Sep 19, 2013 at 12:25 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Thu, Sep 19, 2013 at 11:48 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>> I attached the patch which I have modified.
>
> Thanks for updating the patch!
>
> Here are the review comments:
>
Thank you for reviewing!
> I got the compiler warning:
>
> syncrep.c:112: warning: unused variable 'i'
>
> How does synchronous_transfer work with synchronous_commit?
The currently patch synchronous_transfer doesn't work when
synchronous_commit is set 'off' or 'local'.
if user changes synchronous_commit value on transaction, checkpointer
process can't see it.
Due to that, even if synchronous_commit is changed to 'off' from 'on',
synchronous_transfer doesn't work.
I'm planning to modify the patch so that synchronous_transfer is not
affected by synchronous_commit.
>
> + * accept all the likely variants of "off".
>
> This comment should be removed because synchronous_transfer
> doesn't accept the value "off".
>
> + {"commit", SYNCHRONOUS_TRANSFER_COMMIT, true},
>
> ISTM the third value "true" should be "false".
>
> + {"0", SYNCHRONOUS_TRANSFER_COMMIT, true},
>
> Why is this needed?
>
> + elog(WARNING, "XLogSend sendTimeLineValidUpto(%X/%X) <=
> sentPtr(%X/%X) AND sendTImeLine",
> + (uint32) (sendTimeLineValidUpto >> 32), (uint32)
> sendTimeLineValidUpto,
> + (uint32) (sentPtr >> 32), (uint32) sentPtr);
>
> Why is this needed?
>
They are unnecessary. I had forgot to remove unnecessary codes.
> +#define SYNC_REP_WAIT_FLUSH 1
> +#define SYNC_REP_WAIT_DATA_FLUSH 2
>
> Why do we need to separate the wait-queue for wait-data-flush
> from that for wait-flush? ISTM that wait-data-flush also can
> wait for the replication on the wait-queue for wait-flush, and
> which would simplify the patch.
>
Yes, it seems not necessary to add queue newly.
I will delete SYNC_REP_WAIT_DATA_FLUSH and related that.
Regards,
-------
Sawada Masahiko