Re: Patch for fail-back without fresh backup - Mailing list pgsql-hackers

On Fri, Oct 4, 2013 at 4:32 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> You added several checks into SyncRepWaitForLSN() so that it can handle both
> synchronous_transfer=data_flush and =commit. This change made the source code
> of the function very complicated, I'm afraid. To simplify the source code,
> what about just adding new wait-for-lsn function for data_flush instead of
> changing SyncRepWaitForLSN()? Obviously that new function and
> SyncRepWaitForLSN()
> have the common part. I think that it should be extracted as separate function.

Thank you for reviewing and comment!

yes I agree with you.
I attached the v12 patch which have modified based on above suggestions.
- Added new function SyncRepTransferWaitForLSN() and SyncRepWait()
  SyncRepTransferWaitForLSN() is called on date page flush. OTOH,
SyncRepWatiForLSN() is called on transaction commit.
  And both functions call the SyncRepWait() after check whether sync
commit/transfer is requested.
  Practically server will waits at SyncRepWait().

>
> +     * Note that if sync transfer is requested, we can't regular
> maintenance until
> +     * standbys to connect.
>       */
> -    if (synchronous_commit > SYNCHRONOUS_COMMIT_LOCAL_FLUSH)
> +    if (synchronous_commit > SYNCHRONOUS_COMMIT_LOCAL_FLUSH &&
> !SyncTransRequested())
>
> Per discussion with Pavan, ISTM we don't need to avoid setting
> synchronous_commit
> to local even if synchronous_transfer is data_flush. But you did that here. Why?

I made a mistake. I have removed it.

>
> When synchronous_transfer = data_flush, anti-wraparound vacuum can be blocked.
> Is this safe?
>

In the new version patch, when synchronous_transfer = data_flush/all
AND synchronous_standby_names is set,
vacuum is blocked.
This behaviour of synchronous_transfer similar to synchronous_commit.
Should this allow to do anti-wraparound vacuum even if
synchronous_transfer = data_flush/all?
If so, it also allow to flush the data page while doing vacuum?

> +#synchronous_transfer = commit    # data page synchronization level
> +                # commit, data_flush or all
>
> This comment seems confusing. I think that this parameter specifies when to
> wait for replication.
>
> +typedef enum
> +{
> +    SYNCHRONOUS_TRANSFER_COMMIT,        /* no wait for flush data page */
> +    SYNCHRONOUS_TRANSFER_DATA_FLUSH,    /* wait for data page flush only
> +                                         * no wait for WAL */
> +    SYNCHRONOUS_TRANSFER_ALL            /* wait for data page flush and WAL*/
> +}    SynchronousTransferLevel;
>
> These comments also seem confusing. For example, I think that the meaning of
> SYNCHRONOUS_TRANSFER_COMMIT is something like "wait for replication on
> transaction commit".

Those comments are modified in new patch.

>
> @@ -521,6 +531,13 @@ smgr_redo(XLogRecPtr lsn, XLogRecord *record)
>           */
>          XLogFlush(lsn);
>
> +        /*
> +         * If synchronous transfer is requested, wait for failback safe standby
> +         * to receive WAL up to lsn.
> +         */
> +        if (SyncTransRequested())
> +            SyncRepWaitForLSN(lsn, true, true);
>
> If smgr_redo() is called only during recovery, SyncRepWaitForLSN() doesn't need
> to be called here.

Thank you for info.
I have removed it at smgr_redo().

Regards,

-------
Sawada Masahiko