Re: Patch for fail-back without fresh backup

On Fri, Oct 4, 2013 at 1:46 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Fri, Sep 27, 2013 at 6:44 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Fri, Sep 27, 2013 at 5:18 PM, Pavan Deolasee
>> <pavan(dot)deolasee(at)gmail(dot)com> wrote:
>>> On Fri, Sep 27, 2013 at 1:28 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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