Sorry, the patch attached to the previous main is slightly old. The attached is the correct one.
# They differ only in some phrase in a comment. ==== At Fri, 13 Apr 2018 17:28:40 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp> wrote in <20180413.172840.228724367.horiguchi.kyot...@lab.ntt.co.jp> At Fri, 13 Apr 2018 13:47:51 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp> wrote in <20180413.134751.76149471.horiguchi.kyot...@lab.ntt.co.jp> > At Fri, 13 Apr 2018 08:31:02 +0530, Amit Kapila <amit.kapil...@gmail.com> > wrote in <CAA4eK1LVFpLf=d-7XmfwhLv7Xu53pU0bGU=wvrywsru4xss...@mail.gmail.com> > > On Fri, Apr 13, 2018 at 6:59 AM, Michael Paquier <mich...@paquier.xyz> > > wrote: > > > On Thu, Apr 12, 2018 at 02:55:53PM -0400, Robert Haas wrote: > > >> I think it may actually be confusing. If you run pg_ctl reload and it > > >> reports that the value has changed, you'll expect it to have taken > > >> effect. But really, it will take effect at some later time. > > > > > > > +1. I also think it is confusing and it could be difficult for end > > users to know when the setting is effective. > > > > > It is true that sometimes some people like to temporarily disable > > > full_page_writes particularly when doing some bulk load of data to > > > minimize the effort on WAL, and then re-enable it just after doing > > > the inserting this data. > > > > > > Still does it matter when the change is effective? By disabling > > > full_page_writes even temporarily, you accept the fact that this > > > instance would not be safe until the next checkpoint completes. The > > > instance even finishes by writing less unnecessary WAL data if the > > > change is only effective at the next checkpoint. Well, it is true that > > > this increases potential torn pages problems but the user is already > > > accepting that risk if a crash happens until the next checkpoint then it > > > exposes itself to torn pages anyway as it chose to disable > > > full_page_writes. > > I still don't think that enabling FPW anytime is useful but > disabling seems useful as I mentioned upthread. > > The problem was checkpointer changes the flag anytime including > recovery time. Startup process updates the same flag at the end > of recovery but before publicated. Letting checkpointer change > the flag only at checkpoint time is a straightforward way to > avoid conflicts with startup process. > > I reconsider a bit and came up with the thought that we could > just skip changing shared FPW in checkpointer until recovery > ends, then update the flag after recovery end (perhaps at > checkpoint time in major cases). In this case, FPI is attached > from REDO point of the first checkpoint (not restartpoint) or a > bit earlier, then FPW can be flipped at any time. I'll come up > with that soon. Please find the attached. The most significant change is that UpdateSharedMemoryConfig skips updating of shared fullPageWrites during recovery. The original crash is fixed since this guarantees that XLog working area is initializeed before reaching UpdateFullPageWrites(). Addition to that, I changed CheckpointerMain so that it tries update of shared FPW regardless of SIGHUP and provided new function to just wakeup checkpointer. StartupXLOG wakes up checkpointer either checkpoint is required or not and checkpointer makes the first update of shared FPW at the time. After this point, everything works as the same as the current behavior. > > I think this means that is will be difficult for end users to predict > > unless they track the next checkpoint which isn't too bad, but won't > > be convenient either. > > Looking checkpiont record is enough to know wheter the checkpoint > is protected by FPW eough, but I agree that such strictness is > not crutial. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
>From b17158ce5cb8f29c587fcf8b47ac1ebb8bac6a70 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <horiguchi.kyot...@lab.ntt.co.jp> Date: Fri, 13 Apr 2018 16:29:25 +0900 Subject: [PATCH] Inhibit update shared FPW during recovery The shared full_pages_writes is currently updated by checkpointer even during recovery. This leads to unexpected concurrent updates of the flag or a crash by using uninitialized memory. The update is needless in the first place so this patch inhibits checkpointer from updating it during recovery. Instead, startup process wakes up checkpointer at the end of recovery so as to the shared flag is updated just after. --- src/backend/access/transam/xlog.c | 4 +++ src/backend/postmaster/checkpointer.c | 58 ++++++++++++++++++++++------------- src/include/postmaster/bgwriter.h | 1 + 3 files changed, 42 insertions(+), 21 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 08dc9ba031..d518cc4a60 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -7826,9 +7826,13 @@ StartupXLOG(void) * isn't required for consistency, but the last restartpoint might be far * back, and in case of a crash, recovering from it might take a longer * than is appropriate now that we're not in standby mode anymore. + * If checkpoint is not need, request checkpointer to process shared + * configuration instead. */ if (fast_promoted) RequestCheckpoint(CHECKPOINT_FORCE); + else + WakeupCheckpointer(); } /* diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index 4b452e7cee..3e14f537ef 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -332,12 +332,6 @@ CheckpointerMain(void) */ PG_SETMASK(&UnBlockSig); - /* - * Ensure all shared memory values are set correctly for the config. Doing - * this here ensures no race conditions from other concurrent updaters. - */ - UpdateSharedMemoryConfig(); - /* * Advertise our latch that backends can use to wake us up while we're * sleeping. @@ -368,20 +362,26 @@ CheckpointerMain(void) { got_SIGHUP = false; ProcessConfigFile(PGC_SIGHUP); - - /* - * Checkpointer is the last process to shut down, so we ask it to - * hold the keys for a range of other tasks required most of which - * have nothing to do with checkpointing at all. - * - * For various reasons, some config values can change dynamically - * so the primary copy of them is held in shared memory to make - * sure all backends see the same value. We make Checkpointer - * responsible for updating the shared memory copy if the - * parameter setting changes because of SIGHUP. - */ - UpdateSharedMemoryConfig(); } + + /* + * Checkpointer is the last process to shut down, so we ask it to hold + * the keys for a range of other tasks required most of which have + * nothing to do with checkpointing at all. + * + * For various reasons, some config values can change dynamically so + * the primary copy of them is held in shared memory to make sure all + * backends see the same value. We make Checkpointer responsible for + * updating the shared memory copy if the parameter setting changes + * because of SIGHUP. + * + * Since some of the shared memory copy are in sync only after + * recovery ends, skipped updated are not refrected until the next + * SIGHUP. So call UpdateSharedMemoryConfig() regardless of SIGHUP in + * order to sync them without waiting the next reload. + */ + UpdateSharedMemoryConfig(); + if (checkpoint_requested) { checkpoint_requested = false; @@ -1090,6 +1090,19 @@ RequestCheckpoint(int flags) } } +/* + * WakeupCheckpointer + * + * Requests checkpointer to wake up. This causes per-loop processing + * regardless of the necessity of a checkpoint. + */ +void +WakeupCheckpointer(void) +{ + if (IsUnderPostmaster && CheckpointerShmem->checkpointer_pid != 0) + kill(CheckpointerShmem->checkpointer_pid, SIGINT); +} + /* * ForwardFsyncRequest * Forward a file-fsync request from a backend to the checkpointer @@ -1361,9 +1374,12 @@ UpdateSharedMemoryConfig(void) /* * If full_page_writes has been changed by SIGHUP, we update it in shared - * memory and write an XLOG_FPW_CHANGE record. + * memory and write an XLOG_FPW_CHANGE record. Don't do that during + * recovery since we are not allowed to write WAL and startup process has + * the priority to update the shared flag. */ - UpdateFullPageWrites(); + if (!RecoveryInProgress()) + UpdateFullPageWrites(); elog(DEBUG2, "checkpointer updated shared memory configuration values"); } diff --git a/src/include/postmaster/bgwriter.h b/src/include/postmaster/bgwriter.h index 941c6aba7d..3aac1606a0 100644 --- a/src/include/postmaster/bgwriter.h +++ b/src/include/postmaster/bgwriter.h @@ -29,6 +29,7 @@ extern void BackgroundWriterMain(void) pg_attribute_noreturn(); extern void CheckpointerMain(void) pg_attribute_noreturn(); extern void RequestCheckpoint(int flags); +extern void WakeupCheckpointer(void); extern void CheckpointWriteDelay(int flags, double progress); extern bool ForwardFsyncRequest(RelFileNode rnode, ForkNumber forknum, -- 2.16.3