Re: synchronous_commit = apply - Mailing list pgsql-hackers

On Fri, Sep 18, 2015 at 7:06 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Hello, I have some random comments.

Thanks for the feedback!  I have fixed several of the things that you
found in the attached new version -- see comments inline below.
However, I now know that Simon has a better patch in development to do
this, so I won't be developing this further.  (Until that work is
available, this patch is temporarily useful as a prerequisite for
something else that I'm working on so I'll still be using it...)

> At Wed, 16 Sep 2015 23:07:03 +1200, Thomas Munro <thomas.munro@enterprisedb.com> wrote in
<CAEepm=2_dDqQxgGc83_a48rYza3T4P4vPTpSC6xkHcMEoGyspw@mail.gmail.com>
>> On Tue, Sep 8, 2015 at 1:11 AM, Thomas Munro <thomas.munro@enterprisedb.com>
>> wrote:
>>
>> > On Thu, Sep 3, 2015 at 12:02 AM, Fujii Masao <masao.fujii@gmail.com>
>> >  wrote:
>> > Hmm.  So maybe commit records could have a flag saying 'someone is waiting
>> > for this to commit to apply', and the startup process's apply loop would
>> > only bother to signal the walreceiver if it sees that flag.  I will try
>> > that.
>> >
>>
>> Here is a version that does that, using a bit in xinfo to request apply
>> feedback from standbys when running with synchronous_commit = apply.
>
> The paramter apply_lsn of XLogWalRcvSendReply seems not used in
> the function. Maybe
>
> - applyPtr = GetXLogReplayRecPtr(NULL);
> + applyPtr = apply_lsn != InvalidXLogRecPtr ?
> +                 apply_lsn : GetXLogReplayRecPtr(NULL);

You're right, that is what I meant to do.  Fixed.

> However, walreceiver already sends feedback containing apply lsn
> always so I think it is useless if walreceiver is woke up after
> the commit record is applied.

No, XLogWalRcvSendReply only sends feedback sometimes (see the
conditional early returns).

>> I am not very happy with the way that xact_redo communicates with the main
>> apply loop when it sees that bit, through calls to
>> XLogAppliedSynchronousCommit (essentially a global variable), but I
>> couldn't immediately see a better way to get information out of xact_redo
>> into the apply loop without changing the rm_redo interface.  Perhaps xinfo
>> is the wrong place for that information.  Thoughts?
>
> I think it is better to avoid xact_redo_commit to be involved in
> the standby side mechanism.

I agree that this doesn't seem quite right...

> walreceiver don't seem to be the place to read XLogRecord.
> StartXOG already parses records in recoveryStopsBefore/After. So
> we can do the following thing in place of
> XLogAppliedSynchronousCommit() if additional parsing of xlog
> records in redo loop is acceptable.
>
> XLogImmediatFeedbackAppliedLSN(XLogReaderState *record)
> {
>     if (XLogRecGetRmid(record) != RM_XACT_ID)
>        return false;
>     info = XLogRecGetInfo(record) & XLOG_XACT_OPMASK;
>     if (xact_info != XLOG_XACT_COMMIT &&
>         xact_info != XLOG_XACT_COMMIT_PREPARED)
>         return false;
>     xl_xact_commit *xlrec = (xl_xact_commit *) XLogRecGetData(record);
>     xl_xact_parsed_commit parsed;
>     ParseCommitRecord(XLogRecGetInfo(record), xlrec, &parsed);
>     if (! (parsed->xinfo.xinfo & XACT_XINFO_NEED_APPLY_FEEDBACK))
>         return false;
>
>     WalRcvWakeup();
> }

... but I don't think it's a good idea to parse every commit record
twice.  Maybe there could be an XactGetXinfo function which just takes
reads the xinfo field from the front.

> In WalRcvMain, there's a bit too many if(got_SIGUSR1)'s in the
> main loop.

I agree that this control flow is not ideal, but I won't try to
improve that now that I know that Simon has a patch that doesn't use
signals for this and probably rearranges this loop considerably.

> And the current patch seems to simply double the
> walreceiver reply when got_SIGUSR1.

I don't think so -- the pre-existing call to XLogWalRcvSendReply
doesn't send anything unless certain conditions are met.  You can see
this by testing the first version of the patch I posted in this
thread, which didn't do any of this SIGUSR1 stuff -- in that version,
the test program "test-sync-apply --level apply --loops 5" had to wait
~10 seconds for every commit.

> I found one trival mistake,
>
> --- a/src/backend/replication/syncrep.c
> +++ b/src/backend/replication/syncrep.c
> @@ -462,6 +462,11 @@ SyncRepReleaseWaiters(void)
>          walsndctl->lsn[SYNC_REP_WAIT_FLUSH] = MyWalSnd->flush;
>          numflush = SyncRepWakeQueue(false, SYNC_REP_WAIT_FLUSH);
>      }
> +    if (walsndctl->lsn[SYNC_REP_WAIT_APPLY] < MyWalSnd->apply)
> +    {
> +        walsndctl->lsn[SYNC_REP_WAIT_APPLY] = MyWalSnd->apply;
> +        numflush = SyncRepWakeQueue(false, SYNC_REP_WAIT_APPLY);
> +    }
>
> This overwrites numflush by the value which is to be numapply. So
> the following DEBUG3 message will be wrong.

Oops, right.  Fixed.

--
Thomas Munro
http://www.enterprisedb.com