Hello, I have some random comments.

At Wed, 16 Sep 2015 23:07:03 +1200, Thomas Munro 
<thomas.mu...@enterprisedb.com> wrote in 
<CAEepm=2_ddqqxggc83_a48ryza3t4p4vptpsc6xkhcmeogy...@mail.gmail.com>
> On Tue, Sep 8, 2015 at 1:11 AM, Thomas Munro <thomas.mu...@enterprisedb.com>
> wrote:
> 
> > On Thu, Sep 3, 2015 at 12:02 AM, Fujii Masao <masao.fu...@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);

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.

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

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();
}



In WalRcvMain, there's a bit too many if(got_SIGUSR1)'s in the
main loop. And the current patch seems to simply double the
walreceiver reply when got_SIGUSR1.



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.

> elog(DEBUG3, "released %d procs up to write %X/%X, %d procs up to flush 
> %X/%X",
>      numwrite, (uint32) (MyWalSnd->write >> 32), (uint32) MyWalSnd->write,
>    numflush, (uint32) (MyWalSnd->flush >> 32), (uint32) MyWalSnd->flush);

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to