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

On Fri, Oct 4, 2013 at 1:46 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
> On Fri, Sep 27, 2013 at 6:44 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>> On Fri, Sep 27, 2013 at 5:18 PM, Pavan Deolasee
>> <pavan.deolasee@gmail.com> wrote:
>>> On Fri, Sep 27, 2013 at 1:28 PM, Sawada Masahiko <sawada.mshk@gmail.com>
>>> wrote:
>>>>
>>>>
>>>> >
>>>>
>>>> Thank you for comment. I think it is good simple idea.
>>>> In your opinion, if synchronous_transfer is set 'all' and
>>>> synchronous_commit is set 'on',
>>>> the master wait for data flush eve if user sets synchronous_commit to
>>>> 'local' or 'off'.
>>>> For example, when user want to do transaction early, user can't do this.
>>>> we leave the such situation as constraint?
>>>>
>>>
>>> No, user can still override the transaction commit point wait. So if
>>>
>>> synchronous_transfer is set to "all":
>>>  - If synchronous_commit is ON - wait at all points
>>>  - If synchronous_commit is OFF - wait only at buffer flush (and other
>>> related to failback safety) points
>>>
>>> synchronous_transfer is set to "data_flush":
>>>  - If synchronous_commit is either ON o OFF - do not wait at commit points,
>>> but wait at all other points
>>>
>>> synchronous_transfer is set to "commit":
>>>  - If synchronous_commit is ON - wait at commit point
>>>  - If synchronous_commit is OFF - do not wait at any point
>>>
>>
>> Thank you for explain. Understood.
>> if synchronous_transfer is set 'all' and user changes
>> synchronous_commit to 'off'( or 'local') at a transaction,
>> the master server wait at buffer flush, but doesn't wait at commit
>> points. Right?
>>
>> In currently patch, synchronous_transfer works in cooperation with
>> synchronous_commit.
>> But if user changes synchronous_commit at a transaction, they are not
>> in cooperation.
>> So, your idea might be better than currently behaviour of synchronous_transfer.
>
> I attached the v11 patch which have fixed following contents.

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.

+     * 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?

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

+#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".

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

Regards,

-- 
Fujii Masao