Re: Add checkpoint and redo LSN to LogCheckpointEnd log message - Mailing list pgsql-hackers

Hi, Bharath.

At Tue, 8 Feb 2022 14:33:01 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in 
> On Tue, Feb 8, 2022 at 2:10 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > > Thus.. the attached removes the ambiguity of of the proposed patch
> > > about the LSNs in the restartpoint-ending log message.
> >
> > Thoughts?
> 
> Thanks for the patch. I have few comments on the
> v1-0001-Get-rid-of-unused-path-to-handle-concurrent-check.patch

Thanks for checking this!

> 1) Can we have this Assert right after "skipping restartpoint, already
> performed at %X/%X" error message block? Does it make any difference?
> My point is that if at all, we were to assert this, why can't we do it
> before CheckPointGuts?
> + /* We mustn't have a concurrent checkpoint that advances checkpoint LSN */
> + Assert(lastCheckPoint.redo > ControlFile->checkPointCopy.redo);

No. The assertion checks if something wrong has happend while
CheckPointGuts - which may take a long time - is running.  If we need
to do that, it should be after CheckPointGuts. (However, I removed it
finally. See below.)

> 2) Related to the above Assert, do we really need an assertion or a FATAL error?

It's just to make sure in case where that happens by any chance in
future.  But on second thought, as I mentioned, CreateRestartPoint is
called from standalone process or from checkpointer.  I added that
assertion at the beginning of CreateRestartPoint.  I think that
assertion is logically equal to the old assertion.

I remember that I felt uncomfortable with the lock-less behavior on
ControlFile, which makes the code a bit complex to read.  So I moved
"PriorRedoPtr = " line to within the lock section just below.

Addition to that, I feel being confused by the parallel-use of
lastCheckPoint.redo and RedoRecPtr So I replaced lastCheckPoint.redo
after assiging the former to the latter with RedoRecPtr.

> 3) Let's be consistent with "crash recovery" - replace
> "archive-recovery" with "archive recovery"?
> + * We have exited from archive-recovery mode after this restartpoint
> + * started. Crash recovery ever after should always recover to the end

That's sensible.  I found several existing use of archive-recovery in
xlog.c and a few other files but the fix for them is separated as
another patch (0005).

> 4) Isn't it enough to say "Crash recovery should always recover to the
> end of WAL."?
> + * started. Crash recovery ever after should always recover to the end

I think we need to explicitly say something like "we have exited
archive recovery while *this* restartpoint is running". I simplified
the sentence as the follows.

+  * Archive recovery have ended. Crash recovery ever after should
+  * always recover to the end of WAL.


> 5) Is there a reliable test case covering this code? Please point me
> if the test case is shared upthread somewhere.

Nothing.  Looking from the opposite side, the existing code for the
competing restartpoint/checkpoint case should have not been exercised
at least for these several major versions.  Instead, I added an
assertion at the beginning of CreateRestartPoint that asserts that
"this should be called only under standalone process or from
checkpointer.".  If that assert doesn't fire while the whole test, it
should be the proof of the premise for this patch is correct.

> 6) So, with this patch, the v8 patch-set posted at [1] doesn't need
> any changes IIUC. If that's the case, please feel free to post all the
> patches together such that they get tested in cfbot.

The two are different fixes so I don't think they are ought to be
merged together.

> [1] - https://www.postgresql.org/message-id/CALj2ACUtZhTb%3D2ENkF3BQ3wi137uaGi__qzvXC-qFYC0XwjALw%40mail.gmail.com

As the old 0001, though I said it'fine :p) I added a comment that
reading ControlFile->checkPoint* is safe here.

The old 0002 (attached 0003) looks file to me.

The old 0003 (attached 0004):

+++ b/src/backend/access/rmgrdesc/xlogdesc.c
-        appendStringInfo(buf, "redo %X/%X; "
+        appendStringInfo(buf, "redo lsn %X/%X; "

It is shown in the context of a checkpoint record, so I think it is
not needed or rather lengthning the dump line uselessly. 

+++ b/src/backend/access/transam/xlog.c
-                (errmsg("request to flush past end of generated WAL; request %X/%X, current position %X/%X",
+                (errmsg("request to flush past end of generated WAL; request lsn %X/%X, current lsn %X/%X",

+++ b/src/backend/replication/walsender.c
-                    (errmsg("requested starting point %X/%X is ahead of the WAL flush position of this server %X/%X",
+                    (errmsg("requested starting point %X/%X is ahead of the WAL flush LSN of this server %X/%X",

"WAL" is upper-cased. So it seems rather strange that the "lsn" is
lower-cased.  In the first place the message doesn't look like a
user-facing error message and I feel we don't need position or lsn
there..

+++ b/src/bin/pg_rewind/pg_rewind.c
-        pg_log_info("servers diverged at WAL location %X/%X on timeline %u",
+        pg_log_info("servers diverged at WAL LSN %X/%X on timeline %u",

I feel that we don't need "WAL" there.

+++ b/src/bin/pg_waldump/pg_waldump.c
-    printf(_("  -e, --end=RECPTR       stop reading at WAL location RECPTR\n"));
+    printf(_("  -e, --end=RECPTR       stop reading at WAL LSN RECPTR\n"));

Mmm.. "WAL LSN RECPTR" looks strange to me.  In the first place I
don't think "RECPTR" is a user-facing term. Doesn't something like the
follows work?

+    printf(_("  -e, --end=WAL-LSN       stop reading at WAL-LSN\n"));

In some changes in this patch shorten the main message text of
fprintf-ish functions.  That makes the succeeding parameters can be
inlined.


0001: The fix of CreateRestartPoint
0002: Add LSNs to checkpoint logs (the main patch)
0003: Change "location" to "LSN" of pg_controldata
0004: The same as 0003 for other uses of "location".
0005: Unhyphnate "-recovery" terms.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 9359549b948e6d3a61552985127a86a0a37e83d7 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Tue, 8 Feb 2022 16:42:53 +0900
Subject: [PATCH v9 1/4] Get rid of unused path to handle concurrent
 checkpoints

CreateRestartPoint considered the case a concurrent checkpoint is
running. But 7ff23c6d27 eliminates the possibility that multiple
checkpoints run simultaneously.  That code path, if it were passed,
might leave unrecoverable database by removing WAL segments that are
required by the last established restartpoint.

In passing, the code in the function gets tightened-up and tidied-up
in some points so that it gets easier to read.
---
 src/backend/access/transam/xlog.c     | 64 ++++++++++++++++-----------
 src/backend/postmaster/checkpointer.c |  1 -
 2 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 958220c495..a6c6e67f80 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9659,6 +9659,9 @@ CreateRestartPoint(int flags)
     XLogSegNo    _logSegNo;
     TimestampTz xtime;
 
+    /* we don't assume concurrent checkpoint/restartpoint to run */
+    Assert (!IsUnderPostmaster || MyBackendType == B_CHECKPOINTER);
+
     /* Get a local copy of the last safe checkpoint record. */
     SpinLockAcquire(&XLogCtl->info_lck);
     lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
@@ -9724,7 +9727,7 @@ CreateRestartPoint(int flags)
 
     /* Also update the info_lck-protected copy */
     SpinLockAcquire(&XLogCtl->info_lck);
-    XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+    XLogCtl->RedoRecPtr = RedoRecPtr;
     SpinLockRelease(&XLogCtl->info_lck);
 
     /*
@@ -9743,7 +9746,12 @@ CreateRestartPoint(int flags)
     /* Update the process title */
     update_checkpoint_display(flags, true, false);
 
-    CheckPointGuts(lastCheckPoint.redo, flags);
+    CheckPointGuts(RedoRecPtr, flags);
+
+    /*
+     * Update pg_control, using current time.
+     */
+    LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
 
     /*
      * Remember the prior checkpoint's redo ptr for
@@ -9751,30 +9759,23 @@ CreateRestartPoint(int flags)
      */
     PriorRedoPtr = ControlFile->checkPointCopy.redo;
 
+    ControlFile->checkPoint = lastCheckPointRecPtr;
+    ControlFile->checkPointCopy = lastCheckPoint;
+
     /*
-     * Update pg_control, using current time.  Check that it still shows
-     * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
-     * this is a quick hack to make sure nothing really bad happens if somehow
-     * we get here after the end-of-recovery checkpoint.
+     * Ensure minRecoveryPoint is past the checkpoint record while archive
+     * recovery is still ongoing.  Normally, this will have happened already
+     * while writing out dirty buffers, but not necessarily - e.g. because no
+     * buffers were dirtied.  We do this because a non-exclusive base backup
+     * uses minRecoveryPoint to determine which WAL files must be included in
+     * the backup, and the file (or files) containing the checkpoint record
+     * must be included, at a minimum. Note that for an ordinary restart of
+     * recovery there's no value in having the minimum recovery point any
+     * earlier than this anyway, because redo will begin just after the
+     * checkpoint record.
      */
-    LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
-    if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
-        ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+    if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
     {
-        ControlFile->checkPoint = lastCheckPointRecPtr;
-        ControlFile->checkPointCopy = lastCheckPoint;
-
-        /*
-         * Ensure minRecoveryPoint is past the checkpoint record.  Normally,
-         * this will have happened already while writing out dirty buffers,
-         * but not necessarily - e.g. because no buffers were dirtied.  We do
-         * this because a non-exclusive base backup uses minRecoveryPoint to
-         * determine which WAL files must be included in the backup, and the
-         * file (or files) containing the checkpoint record must be included,
-         * at a minimum. Note that for an ordinary restart of recovery there's
-         * no value in having the minimum recovery point any earlier than this
-         * anyway, because redo will begin just after the checkpoint record.
-         */
         if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
         {
             ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -9786,15 +9787,26 @@ CreateRestartPoint(int flags)
         }
         if (flags & CHECKPOINT_IS_SHUTDOWN)
             ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
-        UpdateControlFile();
     }
+    else
+    {
+        /*
+         * Archive recovery have ended. Crash recovery ever after should always
+         * recover to the end of WAL.
+         */
+        ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+        ControlFile->minRecoveryPointTLI = 0;
+    }
+    UpdateControlFile();
     LWLockRelease(ControlFileLock);
 
     /*
      * Update the average distance between checkpoints/restartpoints if the
      * prior checkpoint exists.
      */
-    if (PriorRedoPtr != InvalidXLogRecPtr)
+
+    /* the second term is just in case */
+    if (PriorRedoPtr != InvalidXLogRecPtr || RedoRecPtr > PriorRedoPtr)
         UpdateCheckPointDistanceEstimate(RedoRecPtr - PriorRedoPtr);
 
     /*
@@ -9864,7 +9876,7 @@ CreateRestartPoint(int flags)
     xtime = GetLatestXTime();
     ereport((log_checkpoints ? LOG : DEBUG2),
             (errmsg("recovery restart point at %X/%X",
-                    LSN_FORMAT_ARGS(lastCheckPoint.redo)),
+                    LSN_FORMAT_ARGS(RedoRecPtr)),
              xtime ? errdetail("Last completed transaction was at log time %s.",
                                timestamptz_to_str(xtime)) : 0));
 
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index 23f691cd47..8bd4c47b6b 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -386,7 +386,6 @@ CheckpointerMain(void)
 
             /* Check if we should perform a checkpoint or a restartpoint. */
             do_restartpoint = RecoveryInProgress();
-
             /*
              * Atomically fetch the request flags to figure out what kind of a
              * checkpoint we should perform, and increase the started-counter
-- 
2.27.0

From 0a8f18e4c13d67d246e39781fadc1717b4f8dcf4 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 1 Feb 2022 04:34:54 +0000
Subject: [PATCH v9 2/4] Add checkpoint and redo LSN to LogCheckpointEnd log
 message

It is useful (for debugging purposes) if the checkpoint end message
has the checkpoint LSN(end) and REDO LSN(start). It gives more
context while analyzing checkpoint-related issues. The pg_controldata
gives the last checkpoint LSN and REDO LSN, but having this info
alongside the log message helps analyze issues that happened
previously, connect the dots and identify the root cause.
---
 src/backend/access/transam/xlog.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index a6c6e67f80..60da34433b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8923,7 +8923,8 @@ LogCheckpointEnd(bool restartpoint)
                         "%d WAL file(s) added, %d removed, %d recycled; "
                         "write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
                         "sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
-                        "distance=%d kB, estimate=%d kB",
+                        "distance=%d kB, estimate=%d kB; "
+                        "lsn=%X/%X, redo lsn=%X/%X",
                         CheckpointStats.ckpt_bufs_written,
                         (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
                         CheckpointStats.ckpt_segs_added,
@@ -8936,14 +8937,18 @@ LogCheckpointEnd(bool restartpoint)
                         longest_msecs / 1000, (int) (longest_msecs % 1000),
                         average_msecs / 1000, (int) (average_msecs % 1000),
                         (int) (PrevCheckPointDistance / 1024.0),
-                        (int) (CheckPointDistanceEstimate / 1024.0))));
+                        (int) (CheckPointDistanceEstimate / 1024.0),
+                        /* we are the only updator of these variables */
+                        LSN_FORMAT_ARGS(ControlFile->checkPoint),
+                        LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
     else
         ereport(LOG,
                 (errmsg("checkpoint complete: wrote %d buffers (%.1f%%); "
                         "%d WAL file(s) added, %d removed, %d recycled; "
                         "write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
                         "sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
-                        "distance=%d kB, estimate=%d kB",
+                        "distance=%d kB, estimate=%d kB; "
+                        "lsn=%X/%X, redo lsn=%X/%X",
                         CheckpointStats.ckpt_bufs_written,
                         (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
                         CheckpointStats.ckpt_segs_added,
@@ -8956,7 +8961,10 @@ LogCheckpointEnd(bool restartpoint)
                         longest_msecs / 1000, (int) (longest_msecs % 1000),
                         average_msecs / 1000, (int) (average_msecs % 1000),
                         (int) (PrevCheckPointDistance / 1024.0),
-                        (int) (CheckPointDistanceEstimate / 1024.0))));
+                        (int) (CheckPointDistanceEstimate / 1024.0),
+                        /* we are the only updator of these variables */
+                        LSN_FORMAT_ARGS(ControlFile->checkPoint),
+                        LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
 }
 
 /*
-- 
2.27.0

From 43d2127235f6d7f58f1daeca9f78afb86bd266f3 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 1 Feb 2022 03:57:04 +0000
Subject: [PATCH v9 3/4] Change "location" to "lsn" in pg_controldata

---
 src/bin/pg_controldata/pg_controldata.c    | 10 +++++-----
 src/test/recovery/t/016_min_consistency.pl |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index f911f98d94..59f39267df 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -235,9 +235,9 @@ main(int argc, char *argv[])
            dbState(ControlFile->state));
     printf(_("pg_control last modified:             %s\n"),
            pgctime_str);
-    printf(_("Latest checkpoint location:           %X/%X\n"),
+    printf(_("Latest checkpoint LSN:                %X/%X\n"),
            LSN_FORMAT_ARGS(ControlFile->checkPoint));
-    printf(_("Latest checkpoint's REDO location:    %X/%X\n"),
+    printf(_("Latest checkpoint's REDO LSN:         %X/%X\n"),
            LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo));
     printf(_("Latest checkpoint's REDO WAL file:    %s\n"),
            xlogfilename);
@@ -274,13 +274,13 @@ main(int argc, char *argv[])
            ckpttime_str);
     printf(_("Fake LSN counter for unlogged rels:   %X/%X\n"),
            LSN_FORMAT_ARGS(ControlFile->unloggedLSN));
-    printf(_("Minimum recovery ending location:     %X/%X\n"),
+    printf(_("Minimum recovery ending LSN:          %X/%X\n"),
            LSN_FORMAT_ARGS(ControlFile->minRecoveryPoint));
     printf(_("Min recovery ending loc's timeline:   %u\n"),
            ControlFile->minRecoveryPointTLI);
-    printf(_("Backup start location:                %X/%X\n"),
+    printf(_("Backup start LSN:                     %X/%X\n"),
            LSN_FORMAT_ARGS(ControlFile->backupStartPoint));
-    printf(_("Backup end location:                  %X/%X\n"),
+    printf(_("Backup end LSN:                       %X/%X\n"),
            LSN_FORMAT_ARGS(ControlFile->backupEndPoint));
     printf(_("End-of-backup record required:        %s\n"),
            ControlFile->backupEndRequired ? _("yes") : _("no"));
diff --git a/src/test/recovery/t/016_min_consistency.pl b/src/test/recovery/t/016_min_consistency.pl
index 86fd6f5546..7ab4d4429d 100644
--- a/src/test/recovery/t/016_min_consistency.pl
+++ b/src/test/recovery/t/016_min_consistency.pl
@@ -126,7 +126,7 @@ my @control_data = split("\n", $stdout);
 my $offline_recovery_lsn = undef;
 foreach (@control_data)
 {
-    if ($_ =~ /^Minimum recovery ending location:\s*(.*)$/mg)
+    if ($_ =~ /^Minimum recovery ending LSN:\s*(.*)$/mg)
     {
         $offline_recovery_lsn = $1;
         last;
-- 
2.27.0

From bb033ba1747a92865afebeff2d48c2ba60c6ab05 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 1 Feb 2022 12:36:34 +0000
Subject: [PATCH v9 4/4] Change "location" to "lsn" in user facing text

---
 src/backend/access/rmgrdesc/xlogdesc.c |  2 +-
 src/backend/access/transam/xlog.c      | 12 ++++++------
 src/backend/replication/repl_scanner.l |  2 +-
 src/backend/replication/walsender.c    |  2 +-
 src/bin/pg_basebackup/pg_basebackup.c  |  6 +++---
 src/bin/pg_basebackup/pg_receivewal.c  |  2 +-
 src/bin/pg_basebackup/pg_recvlogical.c |  4 ++--
 src/bin/pg_basebackup/streamutil.c     |  2 +-
 src/bin/pg_rewind/pg_rewind.c          |  2 +-
 src/bin/pg_rewind/timeline.c           |  2 +-
 src/bin/pg_waldump/pg_waldump.c        | 14 +++++++-------
 src/include/catalog/pg_proc.dat        | 16 ++++++++--------
 12 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index e7452af679..a7ffbd7590 100644
--- a/src/backend/access/rmgrdesc/xlogdesc.c
+++ b/src/backend/access/rmgrdesc/xlogdesc.c
@@ -44,7 +44,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
     {
         CheckPoint *checkpoint = (CheckPoint *) rec;
 
-        appendStringInfo(buf, "redo %X/%X; "
+        appendStringInfo(buf, "redo lsn %X/%X; "
                          "tli %u; prev tli %u; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "
                          "oldest xid %u in DB %u; oldest multi %u in DB %u; "
                          "oldest/newest commit timestamp xid: %u/%u; "
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 60da34433b..acc5ccf174 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1858,7 +1858,7 @@ WaitXLogInsertionsToFinish(XLogRecPtr upto)
     if (upto > reservedUpto)
     {
         ereport(LOG,
-                (errmsg("request to flush past end of generated WAL; request %X/%X, current position %X/%X",
+                (errmsg("request to flush past end of generated WAL; request lsn %X/%X, current lsn %X/%X",
                         LSN_FORMAT_ARGS(upto), LSN_FORMAT_ARGS(reservedUpto))));
         upto = reservedUpto;
     }
@@ -5962,7 +5962,7 @@ recoveryStopsBefore(XLogReaderState *record)
         recoveryStopTime = 0;
         recoveryStopName[0] = '\0';
         ereport(LOG,
-                (errmsg("recovery stopping before WAL location (LSN) \"%X/%X\"",
+                (errmsg("recovery stopping before WAL LSN \"%X/%X\"",
                         LSN_FORMAT_ARGS(recoveryStopLSN))));
         return true;
     }
@@ -6125,7 +6125,7 @@ recoveryStopsAfter(XLogReaderState *record)
         recoveryStopTime = 0;
         recoveryStopName[0] = '\0';
         ereport(LOG,
-                (errmsg("recovery stopping after WAL location (LSN) \"%X/%X\"",
+                (errmsg("recovery stopping after WAL LSN \"%X/%X\"",
                         LSN_FORMAT_ARGS(recoveryStopLSN))));
         return true;
     }
@@ -6715,7 +6715,7 @@ StartupXLOG(void)
      */
     if (!XRecOffIsValid(ControlFile->checkPoint))
         ereport(FATAL,
-                (errmsg("control file contains invalid checkpoint location")));
+                (errmsg("control file contains invalid checkpoint LSN")));
 
     switch (ControlFile->state)
     {
@@ -6843,7 +6843,7 @@ StartupXLOG(void)
                             recoveryTargetName)));
         else if (recoveryTarget == RECOVERY_TARGET_LSN)
             ereport(LOG,
-                    (errmsg("starting point-in-time recovery to WAL location (LSN) \"%X/%X\"",
+                    (errmsg("starting point-in-time recovery to WAL LSN \"%X/%X\"",
                             LSN_FORMAT_ARGS(recoveryTargetLSN))));
         else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
             ereport(LOG,
@@ -6926,7 +6926,7 @@ StartupXLOG(void)
                 if (!ReadRecord(xlogreader, LOG, false,
                                 checkPoint.ThisTimeLineID))
                     ereport(FATAL,
-                            (errmsg("could not find redo location referenced by checkpoint record"),
+                            (errmsg("could not find redo LSN referenced by checkpoint record"),
                              errhint("If you are restoring from a backup, touch \"%s/recovery.signal\" and add
requiredrecovery options.\n"
 
                                      "If you are not restoring from a backup, try removing the file
\"%s/backup_label\".\n"
                                      "Be careful: removing \"%s/backup_label\" will result in a corrupt cluster if
restoringfrom a backup.",
 
diff --git a/src/backend/replication/repl_scanner.l b/src/backend/replication/repl_scanner.l
index d992bcc2e3..f1a435bc22 100644
--- a/src/backend/replication/repl_scanner.l
+++ b/src/backend/replication/repl_scanner.l
@@ -148,7 +148,7 @@ MANIFEST_CHECKSUMS    { return K_MANIFEST_CHECKSUMS; }
                     uint32    hi,
                             lo;
                     if (sscanf(yytext, "%X/%X", &hi, &lo) != 2)
-                        yyerror("invalid streaming start location");
+                        yyerror("invalid streaming start LSN");
                     yylval.recptr = ((uint64) hi) << 32 | lo;
                     return RECPTR;
                 }
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 655760fee3..0c9d86b72c 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -812,7 +812,7 @@ StartReplication(StartReplicationCmd *cmd)
         if (FlushPtr < cmd->startpoint)
         {
             ereport(ERROR,
-                    (errmsg("requested starting point %X/%X is ahead of the WAL flush position of this server %X/%X",
+                    (errmsg("requested starting point %X/%X is ahead of the WAL flush LSN of this server %X/%X",
                             LSN_FORMAT_ARGS(cmd->startpoint),
                             LSN_FORMAT_ARGS(FlushPtr))));
         }
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index c40925c1f0..99b41fc2a6 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -473,7 +473,7 @@ reached_end_position(XLogRecPtr segendpos, uint32 timeline,
 
             if (sscanf(xlogend, "%X/%X", &hi, &lo) != 2)
             {
-                pg_log_error("could not parse write-ahead log location \"%s\"",
+                pg_log_error("could not parse WAL LSN \"%s\"",
                              xlogend);
                 exit(1);
             }
@@ -612,7 +612,7 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
     /* Convert the starting position */
     if (sscanf(startpos, "%X/%X", &hi, &lo) != 2)
     {
-        pg_log_error("could not parse write-ahead log location \"%s\"",
+        pg_log_error("could not parse WAL LSN \"%s\"",
                      startpos);
         exit(1);
     }
@@ -2220,7 +2220,7 @@ BaseBackup(void)
          */
         if (sscanf(xlogend, "%X/%X", &hi, &lo) != 2)
         {
-            pg_log_error("could not parse write-ahead log location \"%s\"",
+            pg_log_error("could not parse WAL LSN \"%s\"",
                          xlogend);
             exit(1);
         }
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index ccb215c398..e8c1631745 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -753,7 +753,7 @@ main(int argc, char **argv)
             case 'E':
                 if (sscanf(optarg, "%X/%X", &hi, &lo) != 2)
                 {
-                    pg_log_error("could not parse end position \"%s\"", optarg);
+                    pg_log_error("could not parse end LSN \"%s\"", optarg);
                     exit(1);
                 }
                 endpos = ((uint64) hi) << 32 | lo;
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index cc35d16f32..0bfb9edb94 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -781,7 +781,7 @@ main(int argc, char **argv)
             case 'I':
                 if (sscanf(optarg, "%X/%X", &hi, &lo) != 2)
                 {
-                    pg_log_error("could not parse start position \"%s\"", optarg);
+                    pg_log_error("could not parse start LSN \"%s\"", optarg);
                     exit(1);
                 }
                 startpos = ((uint64) hi) << 32 | lo;
@@ -789,7 +789,7 @@ main(int argc, char **argv)
             case 'E':
                 if (sscanf(optarg, "%X/%X", &hi, &lo) != 2)
                 {
-                    pg_log_error("could not parse end position \"%s\"", optarg);
+                    pg_log_error("could not parse end LSN \"%s\"", optarg);
                     exit(1);
                 }
                 endpos = ((uint64) hi) << 32 | lo;
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 4a6afd1a06..41d0473138 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -447,7 +447,7 @@ RunIdentifySystem(PGconn *conn, char **sysid, TimeLineID *starttli,
     {
         if (sscanf(PQgetvalue(res, 0, 2), "%X/%X", &hi, &lo) != 2)
         {
-            pg_log_error("could not parse write-ahead log location \"%s\"",
+            pg_log_error("could not parse WAL LSN \"%s\"",
                          PQgetvalue(res, 0, 2));
 
             PQclear(res);
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index efb82a4034..7e4a1c2c30 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -343,7 +343,7 @@ main(int argc, char **argv)
         XLogRecPtr    chkptendrec;
 
         findCommonAncestorTimeline(&divergerec, &lastcommontliIndex);
-        pg_log_info("servers diverged at WAL location %X/%X on timeline %u",
+        pg_log_info("servers diverged at WAL LSN %X/%X on timeline %u",
                     LSN_FORMAT_ARGS(divergerec),
                     targetHistory[lastcommontliIndex].tli);
 
diff --git a/src/bin/pg_rewind/timeline.c b/src/bin/pg_rewind/timeline.c
index df8f82a50c..0b3c524f16 100644
--- a/src/bin/pg_rewind/timeline.c
+++ b/src/bin/pg_rewind/timeline.c
@@ -79,7 +79,7 @@ rewind_parseTimeLineHistory(char *buffer, TimeLineID targetTLI, int *nentries)
         if (nfields != 3)
         {
             pg_log_error("syntax error in history file: %s", fline);
-            pg_log_error("Expected a write-ahead log switchpoint location.");
+            pg_log_error("Expected a WAL switchpoint LSN.");
             exit(1);
         }
         if (entries && tli <= lasttli)
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index a6251e1a96..f792e6351d 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -765,7 +765,7 @@ usage(void)
     printf(_("  %s [OPTION]... [STARTSEG [ENDSEG]]\n"), progname);
     printf(_("\nOptions:\n"));
     printf(_("  -b, --bkp-details      output detailed information about backup blocks\n"));
-    printf(_("  -e, --end=RECPTR       stop reading at WAL location RECPTR\n"));
+    printf(_("  -e, --end=RECPTR       stop reading at WAL LSN RECPTR\n"));
     printf(_("  -f, --follow           keep retrying after reaching end of WAL\n"));
     printf(_("  -n, --limit=N          number of records to display\n"));
     printf(_("  -p, --path=PATH        directory in which to find log segment files or a\n"
@@ -774,7 +774,7 @@ usage(void)
     printf(_("  -q, --quiet            do not print any output, except for errors\n"));
     printf(_("  -r, --rmgr=RMGR        only show records generated by resource manager RMGR;\n"
              "                         use --rmgr=list to list valid resource manager names\n"));
-    printf(_("  -s, --start=RECPTR     start reading at WAL location RECPTR\n"));
+    printf(_("  -s, --start=RECPTR     start reading at WAL LSN RECPTR\n"));
     printf(_("  -t, --timeline=TLI     timeline from which to read log records\n"
              "                         (default: 1 or the value used in STARTSEG)\n"));
     printf(_("  -V, --version          output version information, then exit\n"));
@@ -883,7 +883,7 @@ main(int argc, char **argv)
             case 'e':
                 if (sscanf(optarg, "%X/%X", &xlogid, &xrecoff) != 2)
                 {
-                    pg_log_error("could not parse end WAL location \"%s\"",
+                    pg_log_error("could not parse end WAL LSN \"%s\"",
                                  optarg);
                     goto bad_argument;
                 }
@@ -935,7 +935,7 @@ main(int argc, char **argv)
             case 's':
                 if (sscanf(optarg, "%X/%X", &xlogid, &xrecoff) != 2)
                 {
-                    pg_log_error("could not parse start WAL location \"%s\"",
+                    pg_log_error("could not parse start WAL LSN \"%s\"",
                                  optarg);
                     goto bad_argument;
                 }
@@ -1026,7 +1026,7 @@ main(int argc, char **argv)
             XLogSegNoOffsetToRecPtr(segno, 0, WalSegSz, private.startptr);
         else if (!XLByteInSeg(private.startptr, segno, WalSegSz))
         {
-            pg_log_error("start WAL location %X/%X is not inside file \"%s\"",
+            pg_log_error("start WAL LSN %X/%X is not inside file \"%s\"",
                          LSN_FORMAT_ARGS(private.startptr),
                          fname);
             goto bad_argument;
@@ -1068,7 +1068,7 @@ main(int argc, char **argv)
         if (!XLByteInSeg(private.endptr, segno, WalSegSz) &&
             private.endptr != (segno + 1) * WalSegSz)
         {
-            pg_log_error("end WAL location %X/%X is not inside file \"%s\"",
+            pg_log_error("end WAL LSN %X/%X is not inside file \"%s\"",
                          LSN_FORMAT_ARGS(private.endptr),
                          argv[argc - 1]);
             goto bad_argument;
@@ -1080,7 +1080,7 @@ main(int argc, char **argv)
     /* we don't know what to print */
     if (XLogRecPtrIsInvalid(private.startptr))
     {
-        pg_log_error("no start WAL location given");
+        pg_log_error("no start WAL LSN given");
         goto bad_argument;
     }
 
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7024dbe10a..c302544486 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6299,24 +6299,24 @@
   proname => 'pg_create_restore_point', provolatile => 'v',
   prorettype => 'pg_lsn', proargtypes => 'text',
   prosrc => 'pg_create_restore_point' },
-{ oid => '2849', descr => 'current wal write location',
+{ oid => '2849', descr => 'current wal write lsn',
   proname => 'pg_current_wal_lsn', provolatile => 'v', prorettype => 'pg_lsn',
   proargtypes => '', prosrc => 'pg_current_wal_lsn' },
-{ oid => '2852', descr => 'current wal insert location',
+{ oid => '2852', descr => 'current wal insert lsn',
   proname => 'pg_current_wal_insert_lsn', provolatile => 'v',
   prorettype => 'pg_lsn', proargtypes => '',
   prosrc => 'pg_current_wal_insert_lsn' },
-{ oid => '3330', descr => 'current wal flush location',
+{ oid => '3330', descr => 'current wal flush lsn',
   proname => 'pg_current_wal_flush_lsn', provolatile => 'v',
   prorettype => 'pg_lsn', proargtypes => '',
   prosrc => 'pg_current_wal_flush_lsn' },
 { oid => '2850',
-  descr => 'wal filename and byte offset, given a wal location',
+  descr => 'wal filename and byte offset, given a wal lsn',
   proname => 'pg_walfile_name_offset', prorettype => 'record',
   proargtypes => 'pg_lsn', proallargtypes => '{pg_lsn,text,int4}',
   proargmodes => '{i,o,o}', proargnames => '{lsn,file_name,file_offset}',
   prosrc => 'pg_walfile_name_offset' },
-{ oid => '2851', descr => 'wal filename, given a wal location',
+{ oid => '2851', descr => 'wal filename, given a wal lsn',
   proname => 'pg_walfile_name', prorettype => 'text', proargtypes => 'pg_lsn',
   prosrc => 'pg_walfile_name' },
 
@@ -6332,11 +6332,11 @@
   proname => 'pg_is_in_recovery', provolatile => 'v', prorettype => 'bool',
   proargtypes => '', prosrc => 'pg_is_in_recovery' },
 
-{ oid => '3820', descr => 'current wal flush location',
+{ oid => '3820', descr => 'current wal flush lsn',
   proname => 'pg_last_wal_receive_lsn', provolatile => 'v',
   prorettype => 'pg_lsn', proargtypes => '',
   prosrc => 'pg_last_wal_receive_lsn' },
-{ oid => '3821', descr => 'last wal replay location',
+{ oid => '3821', descr => 'last wal replay lsn',
   proname => 'pg_last_wal_replay_lsn', provolatile => 'v',
   prorettype => 'pg_lsn', proargtypes => '',
   prosrc => 'pg_last_wal_replay_lsn' },
@@ -11509,7 +11509,7 @@
   proparallel => 'r', prorettype => 'void', proargtypes => '',
   prosrc => 'pg_replication_origin_xact_reset' },
 
-{ oid => '6012', descr => 'advance replication origin to specific location',
+{ oid => '6012', descr => 'advance replication origin to specific lsn',
   proname => 'pg_replication_origin_advance', provolatile => 'v',
   proparallel => 'u', prorettype => 'void', proargtypes => 'text pg_lsn',
   prosrc => 'pg_replication_origin_advance' },
-- 
2.27.0

From 7af68e6bbebc52226fbb954e9cff78fca4253097 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Wed, 9 Feb 2022 11:33:47 +0900
Subject: [PATCH v9 5/5] Get rid of uses of some hyphenated words

There are use of both "archive-recovery" and "archive recovery" in the
tree. Unify the similar uses to unhyphenated.  The use of
"point-in-time-recovery" is left alone as it is used as the
explanation for the acronym "PITR".
---
 doc/src/sgml/ref/pg_ctl-ref.sgml   | 2 +-
 doc/src/sgml/ref/postgres-ref.sgml | 2 +-
 src/backend/access/transam/xlog.c  | 2 +-
 src/backend/commands/trigger.c     | 2 +-
 src/backend/utils/cache/relcache.c | 2 +-
 src/test/regress/parallel_schedule | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/ref/pg_ctl-ref.sgml b/doc/src/sgml/ref/pg_ctl-ref.sgml
index 3946fa52ea..7cbe5c3048 100644
--- a/doc/src/sgml/ref/pg_ctl-ref.sgml
+++ b/doc/src/sgml/ref/pg_ctl-ref.sgml
@@ -194,7 +194,7 @@ PostgreSQL documentation
    rolled back and clients are forcibly disconnected, then the
    server is shut down.  <quote>Immediate</quote> mode will abort
    all server processes immediately, without a clean shutdown.  This choice
-   will lead to a crash-recovery cycle during the next server start.
+   will lead to a crash recovery cycle during the next server start.
   </para>
 
   <para>
diff --git a/doc/src/sgml/ref/postgres-ref.sgml b/doc/src/sgml/ref/postgres-ref.sgml
index 55a3f6c69d..fc22fbc4fe 100644
--- a/doc/src/sgml/ref/postgres-ref.sgml
+++ b/doc/src/sgml/ref/postgres-ref.sgml
@@ -719,7 +719,7 @@ PostgreSQL documentation
    is also unwise to send <literal>SIGKILL</literal> to a server
    process — the main <command>postgres</command> process will
    interpret this as a crash and will force all the sibling processes
-   to quit as part of its standard crash-recovery procedure.
+   to quit as part of its standard crash recovery procedure.
   </para>
  </refsect1>
 
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index acc5ccf174..2d5ff5d751 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5691,7 +5691,7 @@ validateRecoveryParameters(void)
 }
 
 /*
- * Exit archive-recovery state
+ * Exit archive recovery state
  */
 static void
 exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog, TimeLineID newTLI)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 1a9c1ac290..d454aa9015 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1887,7 +1887,7 @@ RelationBuildTriggers(Relation relation)
     /*
      * Note: since we scan the triggers using TriggerRelidNameIndexId, we will
      * be reading the triggers in name order, except possibly during
-     * emergency-recovery operations (ie, IgnoreSystemIndexes). This in turn
+     * emergency recovery operations (ie, IgnoreSystemIndexes). This in turn
      * ensures that triggers will be fired in name order.
      */
     ScanKeyInit(&skey,
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 2707fed12f..c14dd91825 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -6560,7 +6560,7 @@ RelationCacheInitFilePostInvalidate(void)
  * Remove the init files during postmaster startup.
  *
  * We used to keep the init files across restarts, but that is unsafe in PITR
- * scenarios, and even in simple crash-recovery cases there are windows for
+ * scenarios, and even in simple crash recovery cases there are windows for
  * the init files to become out-of-sync with the database.  So now we just
  * remove them during startup and expect the first backend launch to rebuild
  * them.  Of course, this has to happen in each database of the cluster.
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 6d8f524ae9..8251744bbf 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -13,7 +13,7 @@ test: test_setup
 
 # run tablespace by itself, and early, because it forces a checkpoint;
 # we'd prefer not to have checkpoints later in the tests because that
-# interferes with crash-recovery testing.
+# interferes with crash recovery testing.
 test: tablespace
 
 # ----------
-- 
2.27.0