At Thu, 30 Aug 2018 18:48:55 -0700, Michael Paquier <mich...@paquier.xyz> wrote
in <20180831014855.gj15...@paquier.xyz>
> On Fri, Aug 31, 2018 at 09:48:46AM +0900, Kyotaro HORIGUCHI wrote:
> > Please wait a bit.. I have a concern about this.
>
> Sure, please feel free.
Thanks.
I looked though the patch and related code and have a concern.
The patch inhibits turning off updateMinRecoveryPoint on other
than the startup process running crash-recovery except at the end
of XLogNeedsFlush.
> /*
> * Check minRecoveryPoint for any other process than the startup
> * process doing crash recovery, which should not update the control
> * file value if crash recovery is still running.
> */
> if (XLogRecPtrIsInvalid(minRecoveryPoint))
> updateMinRecoveryPoint = false;
Even if any other processes than the startup calls the function
during crash recovery, they don't have a means to turn on
updateMinRecoveryPoint again. Actually the only process that
calls the function druing crash recovery is the startup. bwriter
and checkpointer doesn't. Hot-standby backends come after
crash-recvery ends. After all, we just won't have an invalid
minRecoveryPoint value there, and updateMinRecoverypoint must be
true.
Other than that I didn't find a problem. Please find the attached
patch. I also have not come up with how to check the case
automatically..
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 493f1db7b9..74692a128d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2723,9 +2723,13 @@ UpdateMinRecoveryPoint(XLogRecPtr lsn, bool force)
* i.e., we're doing crash recovery. We never modify the control file's
* value in that case, so we can short-circuit future checks here too. The
* local values of minRecoveryPoint and minRecoveryPointTLI should not be
- * updated until crash recovery finishes.
+ * updated until crash recovery finishes. We only do this for the startup
+ * process as it should not update its own reference of minRecoveryPoint
+ * until it has finished crash recovery to make sure that all WAL
+ * available is replayed in this case. This also saves from extra locks
+ * taken on the control file from the startup process.
*/
- if (XLogRecPtrIsInvalid(minRecoveryPoint))
+ if (XLogRecPtrIsInvalid(minRecoveryPoint) && InRecovery)
{
updateMinRecoveryPoint = false;
return;
@@ -2737,7 +2741,9 @@ UpdateMinRecoveryPoint(XLogRecPtr lsn, bool force)
minRecoveryPoint = ControlFile->minRecoveryPoint;
minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
- if (force || minRecoveryPoint < lsn)
+ if (XLogRecPtrIsInvalid(minRecoveryPoint))
+ updateMinRecoveryPoint = false;
+ else if (force || minRecoveryPoint < lsn)
{
XLogRecPtr newMinRecoveryPoint;
TimeLineID newMinRecoveryPointTLI;
@@ -3124,12 +3130,15 @@ XLogNeedsFlush(XLogRecPtr record)
if (RecoveryInProgress())
{
/*
- * An invalid minRecoveryPoint means that we need to recover all the
- * WAL, i.e., we're doing crash recovery. We never modify the control
- * file's value in that case, so we can short-circuit future checks
- * here too.
+ * An invalid minRecoveryPoint on the startup process means that we
+ * need to recover all the WAL, i.e., we're doing crash recovery. We
+ * never modify the control file's value in that case, so we can
+ * short-circuit future checks here too. This triggers a quick exit
+ * path for the startup process, which cannot update its local copy of
+ * minRecoveryPoint as long as it has not replayed all WAL available
+ * when doing crash recovery.
*/
- if (XLogRecPtrIsInvalid(minRecoveryPoint))
+ if (XLogRecPtrIsInvalid(minRecoveryPoint) && InRecovery)
updateMinRecoveryPoint = false;
/* Quick exit if already known to be updated or cannot be updated */
@@ -3146,6 +3155,12 @@ XLogNeedsFlush(XLogRecPtr record)
minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
LWLockRelease(ControlFileLock);
+ /*
+ * No other process than the startup doesn't reach here before crash
+ * recovery ends. So minRecoveryPoint must have a valid value here.
+ */
+ Assert(minRecoveryPoint != InvalidXLogRecPtr);
+
/* check again */
return record > minRecoveryPoint;
}
