Re: Patch for fail-back without fresh backup

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