On Tue, Sep 18, 2018 at 04:15:42PM +0900, Kyotaro HORIGUCHI wrote: > My latest patch tries to remove the window by imposing all > responsibility to apply config file changes to the shared FPW > flag on the checkpointer. RecoveryInProgress() is changed to be > called prior to UpdateFullPageWrites on the way doing that.
I still need to look at that in details. That may be better than what I
am proposing. At quick glance what I propose is more simple, and does
not need enforcing a checkpoint using SIGINT.
> InRecoery is turned off after the last UpdateFullPageWrite() and
> before SharedRecoveryInProgress is turned off. So it is still
> leaving the window. Thus, checkpointer stops flipping the value
> before SharedRecoveryInProgress's turning off. (I don't
> understand InRecovery condition..) However, this lets
> checkpointer omit reloading after UpdateFullPagesWrite() in
> startup until SharedRecoveryInProgress is tunred off.
That won't matter in this case, as RecoveryInProgress() gets called out
of the critical section in the previous patch I sent, so there is no
failure. Let's not forget that the first issue is the allocation in the
critical section. The second issue is that UpdateFullPageWrites may be
called concurrently across multiple processes, which is not something it
is designed for. The second issue gets addressed in my proposal my
making sure that the checkpointer never tries to update full_page_writes
by himself until recovery has finished, and that the startup process is
the only updater once recovery ends.
> Agreed. "If we need to do that in the start process," we need to
> change the shared flag and issue FPW_CHANGE always when the
> database state is different from configuration file, regardless
> of what StartXLOG() did until the point.
Definitely my mistake here. Attached is a patch to show what I have in
mind by making sure that the startup process generates a record even
after switching full_page_writes when started normally. This removes
the condition based on InRecovery, and uses a new argument for
UpdateFullPageWrites() instead. Your test case,as well as what I do
manually for testing, pass without triggering the assertion.
+ /* DEBUG: cause a reload */
+ {
+ struct stat b;
+ if (stat("/tmp/hoge", &b) == 0)
+ {
+ elog(LOG, "STARTUP SLEEP FOR 1s");
+ sleep(1);
+ elog(LOG, "DONE.");
+ DirectFunctionCall1(pg_reload_conf, 0);
+ }
+ }
The way you patch the backend this way is always nice to see so as it is
easy to reproduce bugs, and it actually helps in reproducing the
assertion failure correctly ;)
--
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 3025d0badb..079e1814c1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7719,10 +7719,15 @@ StartupXLOG(void)
* Update full_page_writes in shared memory and write an XLOG_FPW_CHANGE
* record before resource manager writes cleanup WAL records or checkpoint
* record is written.
+ *
+ * It is safe to check the shared full_page_writes without the lock,
+ * because there is no concurrently running process able to update it.
+ * The only other process able to update full_page_writes is the
+ * checkpointer, still it is unable to do so until recovery finishes.
*/
Insert->fullPageWrites = lastFullPageWrites;
LocalSetXLogInsertAllowed();
- UpdateFullPageWrites();
+ UpdateFullPageWrites(true);
LocalXLogInsertAllowed = -1;
if (InRecovery)
@@ -9693,14 +9698,28 @@ XLogReportParameters(void)
* Update full_page_writes in shared memory, and write an
* XLOG_FPW_CHANGE record if necessary.
*
- * Note: this function assumes there is no other process running
- * concurrently that could update it.
+ * Note: this can be called from the checkpointer, or the startup process
+ * at the end of recovery. One could think that this routine should be
+ * careful with its lock handling, however this is a no-op for the
+ * checkpointer until the startup process marks the end of recovery,
+ * so only one of them can do the work of this routine at once.
*/
void
-UpdateFullPageWrites(void)
+UpdateFullPageWrites(bool force)
{
XLogCtlInsert *Insert = &XLogCtl->Insert;
+ /*
+ * Check if recovery is still in progress before entering this critical
+ * section, as some memory allocation could happen at the end of
+ * recovery. There is nothing to do for a system still in recovery.
+ * Note that the caller may still want to enforce an update to happen.
+ * One such example is the startup process, which updates full_page_writes
+ * at the end of recovery.
+ */
+ if (RecoveryInProgress() && !force)
+ return;
+
/*
* Do nothing if full_page_writes has not been changed.
*
@@ -9731,7 +9750,7 @@ UpdateFullPageWrites(void)
* Write an XLOG_FPW_CHANGE record. This allows us to keep track of
* full_page_writes during archive recovery, if required.
*/
- if (XLogStandbyInfoActive() && !RecoveryInProgress())
+ if (XLogStandbyInfoActive())
{
XLogBeginInsert();
XLogRegisterData((char *) (&fullPageWrites), sizeof(bool));
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index 1a033093c5..2084978468 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -1347,7 +1347,7 @@ UpdateSharedMemoryConfig(void)
* If full_page_writes has been changed by SIGHUP, we update it in shared
* memory and write an XLOG_FPW_CHANGE record.
*/
- UpdateFullPageWrites();
+ UpdateFullPageWrites(false);
elog(DEBUG2, "checkpointer updated shared memory configuration values");
}
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 421ba6d775..7cfed58399 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -270,7 +270,7 @@ extern void CreateCheckPoint(int flags);
extern bool CreateRestartPoint(int flags);
extern void XLogPutNextOid(Oid nextOid);
extern XLogRecPtr XLogRestorePoint(const char *rpName);
-extern void UpdateFullPageWrites(void);
+extern void UpdateFullPageWrites(bool force);
extern void GetFullPageWriteInfo(XLogRecPtr *RedoRecPtr_p, bool *doPageWrites_p);
extern XLogRecPtr GetRedoRecPtr(void);
extern XLogRecPtr GetInsertRecPtr(void);
signature.asc
Description: PGP signature
