Subject: Re: cleanup patches for incremental backup

From: Robert Haas <[email-masked]>
Date: Mon, 15 Jan 2024 11:58:04 -0500

On Sat, Jan 13, 2024 at 1:00 PM Alexander Lakhin <[email-masked]> wrote:
> I've found one more typo in the sgml:
> summarized_pid
> And one in a comment:
> sumamry
>
> A trivial fix is attached.

Thanks, committed.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Matthias van de Meent <[email-masked]>
Date: Mon, 15 Jan 2024 21:31:18 +0100

> > A trivial fix is attached.

Off-list I was notified that the new WAL summarizer process was not
yet added to the glossary, so PFA a patch that does that.
In passing, it also adds "incremental backup" to the glossary, and
updates the documented types of backends in monitoring.sgml with the
new backend type, too.

Kind regards,

Matthias van de Meent.

---

From: Robert Haas <[email-masked]>
Date: Tue, 16 Jan 2024 10:39:10 -0500

> updates the documented types of backends in monitoring.sgml with the

I wonder if it's possible that you sent the wrong version of this
patch, because:

(1) The docs don't build with this applied. I'm not sure if it's the
only problem, but <glossterm linkend="glossary-db-cluster" is missing
the closing >.

(2) The changes to monitoring.sgml contain an unrelated change, about
pg_stat_all_indexes.idx_scan.

Also, I think the "For more information, see <xref linkend="whatever"
/> bit should have a period after the markup tag, as we seem to do in
other cases.

One other thought is that the incremental backup only replaces
relation files with incremental files, and it never does anything
about FSM files. So the statement that it only contains data that was
potentially changed isn't quite correct. It might be better to phrase
it the other way around i.e. it is like a full backup, except that
some files can be replaced by incremental files which omit blocks to
which no WAL-logged changes have been made.

---

From: Matthias van de Meent <[email-masked]>
Date: Tue, 16 Jan 2024 21:22:15 +0100

> only problem, but <glossterm linkend="glossary-db-cluster" is missing

That's my mistake, I didn't check install-world yet due to unrelated
issues building the docs. I've since sorted out these issues (this was
a good stick to get that done), so this issue is fixed in the attached
patch.

> pg_stat_all_indexes.idx_scan.

Thanks for noticing, fixed in attached.

> /> bit should have a period after the markup tag, as we seem to do in

Fixed.

> which no WAL-logged changes have been made.

How about the attached?

Kind regards,

Matthias van de Meent

---

From: Robert Haas <[email-masked]>
Date: Tue, 16 Jan 2024 15:49:20 -0500

> > which no WAL-logged changes have been made.

I like the direction.

+     A special <glossterm linkend="glossary-basebackup">base backup</glossterm>
+     that for some WAL-logged relations only contains the pages that were
+     modified since a previous backup, as opposed to the full relation data of
+     normal base backups. Like base backups, it is generated by the tool
+     <xref linkend="app-pgbasebackup"/>.

Could we say "that for some files may contain only those pages that
were modified since a previous backup, as opposed to the full contents
of every file"? My thoughts are (1) there's no hard guarantee that an
incremental backup will replace even one file with an incremental
file, although in practice it is probably almost always going to
happen and (2) pg_combinebackup would actually be totally fine with
any file at all being sent incrementally; it's only that the server
isn't smart enough to figure out how to do this with e.g. SLRU data
right now.

+     To restore incremental backups the tool <xref
linkend="app-pgcombinebackup"/>
+     is used, which combines the incremental backups with a base backup and
+     <glossterm linkend="glossary-wal">WAL</glossterm> to restore a
+     <glossterm linkend="glossary-db-cluster">database cluster</glossterm> to
+     a consistent state.

I wondered if this needed to be clearer that the chain of backups
could have length > 2. But on further reflection, I think it's fine,
unless you feel otherwise.

The rest LGTM.

---

From: Matthias van de Meent <[email-masked]>
Date: Wed, 17 Jan 2024 19:42:37 +0100

On Tue, 16 Jan 2024 at 21:49, Robert Haas <[email-masked]> wrote:
> of every file"?

Sure, added in attached.

> +     To restore incremental backups the tool <pgcombinebackup>
> + [...]

I removed "the" from the phrasing "the incremental backups", which
makes it a bit less restricted.

> The rest LGTM.

In the latest patch I also fixed the casing of "Incremental Backup" to
"... backup", to be in line with most other multi-word items.

Thanks!

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

---

From: Robert Haas <[email-masked]>
Date: Wed, 17 Jan 2024 15:10:13 -0500

> Sure, added in attached.

I think this mostly looks good now but I just realized that I think
this needs rephrasing:

+     is used, which combines incremental backups with a base backup and

The way this is worded, at least to me, it makes it sound like
pg_combinebackup is going to do the WAL recovery for you, which it
isn't. Maybe:

To restore incremental backups the tool <xref
linkend="app-pgcombinebackup"/> is used, which combines incremental
backups with a base backup. Afterwards, recovery can use <glossterm
linkend="glossary-wal">WAL</glossterm> to bring the <glossterm
linkend="glossary-db-cluster">database cluster</glossterm> to a
consistent state.

---

From: Matthias van de Meent <[email-masked]>
Date: Thu, 18 Jan 2024 10:49:48 +0100

> linkend="glossary-db-cluster">database cluster</glossterm> to a

Sure, that's fine with me.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

---

From: Robert Haas <[email-masked]>
Date: Thu, 18 Jan 2024 09:40:56 -0500

> Sure, that's fine with me.

OK, committed that way.

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 24 Jan 2024 11:08:46 -0600

I'm seeing some recent buildfarm failures for pg_walsummary:

	https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sungazer&dt=2024-01-14%2006%3A21%3A58
	https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=idiacanthus&dt=2024-01-17%2021%3A10%3A36
	https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=serinus&dt=2024-01-20%2018%3A58%3A49
	https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=taipan&dt=2024-01-23%2002%3A46%3A57
	https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=serinus&dt=2024-01-23%2020%3A23%3A36

The signature looks nearly identical in each:

	#   Failed test 'WAL summary file exists'
	#   at t/002_blocks.pl line 79.

	#   Failed test 'stdout shows block 0 modified'
	#   at t/002_blocks.pl line 85.
	#                   ''
	#     doesn't match '(?^m:FORK main: block 0$)'

I haven't been able to reproduce the issue on my machine, and I haven't
figured out precisely what is happening yet, but I wanted to make sure
there is awareness.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Robert Haas <[email-masked]>
Date: Wed, 24 Jan 2024 12:46:16 -0500

> figured out precisely what is happening yet, but I wanted to make sure

This is weird. There's a little more detail in the log file,
regress_log_002_blocks, e.g. from the first failure you linked:

[11:18:20.683](96.787s) # before insert, summarized TLI 1 through 0/14E09D0
[11:18:21.188](0.505s) # after insert, summarized TLI 1 through 0/14E0D08
[11:18:21.326](0.138s) # examining summary for TLI 1 from 0/14E0D08 to 0/155BAF0
# 1
...
[11:18:21.349](0.000s) #          got: 'pg_walsummary: error: could
not open file "/home/nm/farm/gcc64/HEAD/pgsql.build/src/bin/pg_walsummary/tmp_check/t_002_blocks_node1_data/pgdata/pg_wal/summaries/0000000100000000014E0D0800000000155BAF0
# 1.summary": No such file or directory'

The "examining summary" line is generated based on the output of
pg_available_wal_summaries(). The way that works is that the server
calls readdir(), disassembles the filename into a TLI and two LSNs,
and returns the result. Then, a fraction of a second later, the test
script reassembles those components into a filename and finds the file
missing. If the logic to translate between filenames and TLIs & LSNs
were incorrect, the test would fail consistently. So the only
explanation that seems to fit the facts is the file disappearing out
from under us. But that really shouldn't happen. We do have code to
remove such files in MaybeRemoveOldWalSummaries(), but it's only
supposed to be nuking files more than 10 days old.

So I don't really have a theory here as to what could be happening. :-(

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 24 Jan 2024 12:05:15 -0600

> So I don't really have a theory here as to what could be happening. :-(

There might be an overflow risk in the cutoff time calculation, but I doubt
that's the root cause of these failures:

	/*
	 * Files should only be removed if the last modification time precedes the
	 * cutoff time we compute here.
	 */
	cutoff_time = time(NULL) - 60 * wal_summary_keep_time;

Otherwise, I think we'll probably need to add some additional logging to
figure out what is happening...

---

From: Robert Haas <[email-masked]>
Date: Wed, 24 Jan 2024 14:08:08 -0500

> figure out what is happening...

Where, though? I suppose we could:

1. Change the server code so that it logs each WAL summary file
removed at a log level high enough to show up in the test logs.

2. Change the TAP test so that it prints out readdir(WAL summary
directory) at various points in the test.

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 24 Jan 2024 13:39:22 -0600

> directory) at various points in the test.

That seems like a reasonable starting point.  Even if it doesn't help
determine the root cause, it should at least help rule out concurrent
summary removal.

---

From: Robert Haas <[email-masked]>
Date: Thu, 25 Jan 2024 10:06:41 -0500

> determine the root cause, it should at least help rule out concurrent

Here is a patch for that.

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 25 Jan 2024 10:08:50 -0600

> Here is a patch for that.

LGTM.  The only thing I might add is the cutoff_time in that LOG.

---

From: Robert Haas <[email-masked]>
Date: Fri, 26 Jan 2024 11:04:37 -0500

> LGTM.  The only thing I might add is the cutoff_time in that LOG.

Here is v2 with that addition.

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 26 Jan 2024 11:39:04 -0600

> Here is v2 with that addition.

Looks reasonable.

---

From: Robert Haas <[email-masked]>
Date: Fri, 26 Jan 2024 13:37:41 -0500

> > Here is v2 with that addition.

Thanks for the report & review. I have committed that version.

---

From: Alexander Lakhin <[email-masked]>
Date: Sat, 27 Jan 2024 10:00:01 +0300

Hello Robert,

> Thanks for the report & review. I have committed that version.

While trying to reproduce the 002_blocks test failure, I've encountered
another anomaly (or two):
make -s check -C src/bin/pg_walsummary/ PROVE_TESTS="t/002*" PROVE_FLAGS="--timer"
# +++ tap check in src/bin/pg_walsummary +++
[05:40:38] t/002_blocks.pl .. # poll_query_until timed out executing this query:
# SELECT EXISTS (
#     SELECT * from pg_available_wal_summaries()
#     WHERE tli = 0 AND end_lsn > '0/0'
# )
#
# expecting this output:
# t
# last actual query output:
# f
# with stderr:
[05:40:38] t/002_blocks.pl .. ok   266739 ms ( 0.00 usr  0.01 sys + 17.51 cusr 26.79 csys = 44.31 CPU)
[05:45:05]
All tests successful.
Files=1, Tests=3, 267 wallclock secs ( 0.02 usr  0.02 sys + 17.51 cusr 26.79 csys = 44.34 CPU)
Result: PASS

It looks like the test may call pg_get_wal_summarizer_state() when
WalSummarizerCtl->initialized is false yet, i. e. before the first
GetOldestUnsummarizedLSN() call.
I could reproduce the issue easily (within 10 test runs) with
pg_usleep(100000L);
added inside WalSummarizerMain() just below:
sigprocmask(SIG_SETMASK, &UnBlockSig, NULL);

But the fact that the test passes regardless of the timeout, make me
wonder, whether any test should fail when such timeout occurs?

Best regards,
Alexander

---

From: Alexander Lakhin <[email-masked]>
Date: Sat, 27 Jan 2024 11:00:01 +0300

24.01.2024 20:46, Robert Haas wrote:
> and returns the result.

I'm discouraged by "\n1" in the file name and in the
"examining summary..." message.
regress_log_002_blocks from the following successful test run on the same
sungazer node contains:
[15:21:58.924](0.106s) # examining summary for TLI 1 from 0/155BAE0 to 0/155E750
[15:21:58.925](0.001s) ok 1 - WAL summary file exists

Best regards,
Alexander

---

From: Nathan Bossart <[email-masked]>
Date: Sat, 27 Jan 2024 10:31:09 -0600

> [15:21:58.925](0.001s) ok 1 - WAL summary file exists

Ah, I think this query:

	SELECT tli, start_lsn, end_lsn from pg_available_wal_summaries()
		WHERE tli = $summarized_tli AND end_lsn > '$summarized_lsn'

is returning more than one row in some cases.  I attached a quick sketch of
an easy way to reproduce the issue as well as one way to fix it.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/src/bin/pg_walsummary/t/002_blocks.pl b/src/bin/pg_walsummary/t/002_blocks.pl
index 40908da8cb..5609b1bd14 100644
--- a/src/bin/pg_walsummary/t/002_blocks.pl
+++ b/src/bin/pg_walsummary/t/002_blocks.pl
@@ -64,10 +64,16 @@ SELECT EXISTS (
 )
 EOM
 
+$node1->safe_psql('postgres', <<EOM);
+UPDATE mytable SET b = 'abefghijklmnopqrstuvwxyz' WHERE a = 2;
+CHECKPOINT;
+EOM
+$node1->safe_psql('postgres', 'SELECT pg_sleep(1);');
+
 # Figure out the exact details for the new summary file.
 my $details = $node1->safe_psql('postgres', <<EOM);
 SELECT tli, start_lsn, end_lsn from pg_available_wal_summaries()
-	WHERE tli = $summarized_tli AND end_lsn > '$summarized_lsn'
+	WHERE tli = $summarized_tli AND end_lsn > '$summarized_lsn' ORDER BY end_lsn LIMIT 1
 EOM
 my ($tli, $start_lsn, $end_lsn) = split(/\|/, $details);
 note("examining summary for TLI $tli from $start_lsn to $end_lsn");

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 29 Jan 2024 12:21:30 -0600

> an easy way to reproduce the issue as well as one way to fix it.

The buildfarm just caught a failure with the new logging in place:

	https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=calliphoridae&dt=2024-01-29%2018%3A09%3A10

I'm not totally sure my "fix" is correct, but I think this does confirm the
theory.

---

From: Robert Haas <[email-masked]>
Date: Mon, 29 Jan 2024 15:18:50 -0500

> I'm not totally sure my "fix" is correct, but I think this does confirm the

Ah. The possibilities of ending up with TWO new WAL summaries never
occurred to me. Things that never occurred to the developer are a
leading cause of bugs, and so here.

I'm wondering if what we need to do is run pg_walsummary on both
summary files in that case. If we just pick one or the other, how do
we know which one to pick?

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 29 Jan 2024 15:13:21 -0600

> we know which one to pick?

Even if we do that, isn't it possible that none of the summaries will
include the change?  Presently, we get the latest summarized LSN, make a
change, and then wait for the next summary file with a greater LSN than
what we saw before the change.  But AFAICT there's no guarantee that means
the change has been summarized yet, although the chances of that happening
in a test are probably pretty small.

Could we get the LSN before and after making the change and then inspect
all summaries that include that LSN range?

---

From: Robert Haas <[email-masked]>
Date: Tue, 30 Jan 2024 10:51:26 -0500

> all summaries that include that LSN range?

The trick here is that each WAL summary file covers one checkpoint
cycle. The intent of the test is to load data into the table,
checkpoint, see what summaries exist, then update a row, checkpoint
again, and see what summaries now exist. We expect one new summary
because there's been one new checkpoint. When I was thinking about
this yesterday, I was imagining that we were somehow getting an extra
checkpoint in some cases. But it looks like it's actually an
off-by-one situation. In
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=calliphoridae&dt=2024-01-29%2018%3A09%3A10
the new files that show up between "after insert" and "after new
summary" are:

00000001000000000152FAE000000000015AAAC8.summary (LSN distance ~500k)
00000001000000000152F7A8000000000152FAE0.summary (LSN distance 824 bytes)

The checkpoint after the inserts says:

LOG:  checkpoint complete: wrote 14 buffers (10.9%); 0 WAL file(s)
added, 0 removed, 0 recycled; write=0.956 s, sync=0.929 s, total=3.059
s; sync files=39, longest=0.373 s, average=0.024 s; distance=491 kB,
estimate=491 kB; lsn=0/15AAB20, redo lsn=0/15AAAC8

And the checkpoint after the single-row update says:

LOG:  checkpoint complete: wrote 4 buffers (3.1%); 0 WAL file(s)
added, 0 removed, 0 recycled; write=0.648 s, sync=0.355 s, total=2.798
s; sync files=3, longest=0.348 s, average=0.119 s; distance=11 kB,
estimate=443 kB; lsn=0/15AD770, redo lsn=0/15AD718

So both of the new WAL summary files that are appearing here are from
checkpoints that happened before the single-row update. The larger
file is the one covering the 400 inserts, and the smaller one is the
checkpoint before that. Which means that the "Wait for a new summary
to show up." code isn't actually waiting long enough, and then the
whole thing goes haywire. The problem is, I think, that this code
naively thinks it can just wait for summarized_lsn and everything will
be fine ... but that assumes we were caught up when we first measured
the summarized_lsn, and that need not be so, because it takes some
short but non-zero amount of time for the summarizer to catch up with
the WAL generated during initdb.

I think the solution here is to find a better way to wait for the
inserts to be summarized, one that actually does wait for that to
happen.

---

From: Robert Haas <[email-masked]>
Date: Tue, 30 Jan 2024 11:52:54 -0500

> inserts to be summarized, one that actually does wait for that to

Here's a patch for that. I now think
a7097ca630a25dd2248229f21ebce4968d85d10a was actually misguided, and
served only to mask some of the failures caused by waiting for the WAL
summary file.

---

From: Robert Haas <[email-masked]>
Date: Wed, 31 Jan 2024 10:33:02 -0500

> served only to mask some of the failures caused by waiting for the WAL

Committed.

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 14 Mar 2024 16:00:10 -0500

> 	cutoff_time = time(NULL) - 60 * wal_summary_keep_time;

I've attached a short patch for fixing this overflow risk.  Specifically,
it limits wal_summary_keep_time to INT_MAX / SECS_PER_MINUTE, just like
log_rotation_age.

I considering checking for overflow when we subtract the keep-time from the
result of time(2), but AFAICT there's only a problem if time_t is unsigned,
which Wikipedia leads me to believe is unusual [0], so I figured we might
be able to just wait this one out until 2038.

> figure out what is happening...

Separately, I suppose it's probably time to revert the temporary debugging
code adding by commit 5ddf997.  I can craft a patch for that, too.

[0] https://en.wikipedia.org/wiki/Unix_time#Representing_the_number

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c
index ec2874c18c..c820d1f9ed 100644
--- a/src/backend/postmaster/walsummarizer.c
+++ b/src/backend/postmaster/walsummarizer.c
@@ -139,7 +139,7 @@ static XLogRecPtr redo_pointer_at_last_summary_removal = InvalidXLogRecPtr;
  * GUC parameters
  */
 bool		summarize_wal = false;
-int			wal_summary_keep_time = 10 * 24 * 60;
+int			wal_summary_keep_time = 10 * HOURS_PER_DAY * MINS_PER_HOUR;
 
 static void WalSummarizerShutdown(int code, Datum arg);
 static XLogRecPtr GetLatestLSN(TimeLineID *tli);
@@ -1474,7 +1474,7 @@ MaybeRemoveOldWalSummaries(void)
 	 * Files should only be removed if the last modification time precedes the
 	 * cutoff time we compute here.
 	 */
-	cutoff_time = time(NULL) - 60 * wal_summary_keep_time;
+	cutoff_time = time(NULL) - wal_summary_keep_time * SECS_PER_MINUTE;
 
 	/* Get all the summaries that currently exist. */
 	wslist = GetWalSummaries(0, InvalidXLogRecPtr, InvalidXLogRecPtr);
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 57d9de4dd9..1e71e7db4a 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -3293,9 +3293,9 @@ struct config_int ConfigureNamesInt[] =
 			GUC_UNIT_MIN,
 		},
 		&wal_summary_keep_time,
-		10 * 24 * 60,			/* 10 days */
+		10 * HOURS_PER_DAY * MINS_PER_HOUR, /* 10 days */
 		0,
-		INT_MAX,
+		INT_MAX / SECS_PER_MINUTE,
 		NULL, NULL, NULL
 	},

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 14 Mar 2024 20:52:55 -0500

> code adding by commit 5ddf997.  I can craft a patch for that, too.

As promised...

>From 3923e30acb1d4e21ce311b25edbcd8b96b4223d2 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Thu, 14 Mar 2024 20:36:48 -0500
Subject: [PATCH v1 1/2] Revert "Temporary patch to help debug pg_walsummary
 test failures."

This reverts commit 5ddf9973477729cf161b4ad0a1efd52f4fea9c88.
---
 src/backend/backup/walsummary.c       |  7 -------
 src/bin/pg_walsummary/t/002_blocks.pl | 14 --------------
 2 files changed, 21 deletions(-)

diff --git a/src/backend/backup/walsummary.c b/src/backend/backup/walsummary.c
index 4d047e1c02..322ae3c3ad 100644
--- a/src/backend/backup/walsummary.c
+++ b/src/backend/backup/walsummary.c
@@ -252,15 +252,8 @@ RemoveWalSummaryIfOlderThan(WalSummaryFile *ws, time_t cutoff_time)
 		ereport(ERROR,
 				(errcode_for_file_access(),
 				 errmsg("could not stat file \"%s\": %m", path)));
-	/* XXX temporarily changed to debug buildfarm failures */
-#if 0
 	ereport(DEBUG2,
 			(errmsg_internal("removing file \"%s\"", path)));
-#else
-	ereport(LOG,
-			(errmsg_internal("removing file \"%s\" cutoff_time=%llu", path,
-							 (unsigned long long) cutoff_time)));
-#endif
 }
 
 /*
diff --git a/src/bin/pg_walsummary/t/002_blocks.pl b/src/bin/pg_walsummary/t/002_blocks.pl
index d4bae3d564..52d3bd8840 100644
--- a/src/bin/pg_walsummary/t/002_blocks.pl
+++ b/src/bin/pg_walsummary/t/002_blocks.pl
@@ -51,7 +51,6 @@ my $summarized_lsn = $node1->safe_psql('postgres', <<EOM);
 SELECT MAX(end_lsn) AS summarized_lsn FROM pg_available_wal_summaries()
 EOM
 note("after insert, summarized through $summarized_lsn");
-note_wal_summary_dir("after insert", $node1);
 
 # Update a row in the first block of the table and trigger a checkpoint.
 $node1->safe_psql('postgres', <<EOM);
@@ -78,7 +77,6 @@ my @lines = split(/\n/, $details);
 is(0+@lines, 1, "got exactly one new WAL summary");
 my ($tli, $start_lsn, $end_lsn) = split(/\|/, $lines[0]);
 note("examining summary for TLI $tli from $start_lsn to $end_lsn");
-note_wal_summary_dir("after new summary", $node1);
 
 # Reconstruct the full pathname for the WAL summary file.
 my $filename = sprintf "%s/pg_wal/summaries/%08s%08s%08s%08s%08s.summary",
@@ -86,7 +84,6 @@ my $filename = sprintf "%s/pg_wal/summaries/%08s%08s%08s%08s%08s.summary",
 					   split(m@/@, $start_lsn),
 					   split(m@/@, $end_lsn);
 ok(-f $filename, "WAL summary file exists");
-note_wal_summary_dir("after existence check", $node1);
 
 # Run pg_walsummary on it. We expect exactly two blocks to be modified,
 # block 0 and one other.
@@ -96,16 +93,5 @@ note($stdout);
 like($stdout, qr/FORK main: block 0$/m, "stdout shows block 0 modified");
 is($stderr, '', 'stderr is empty');
 is(0+@lines, 2, "UPDATE modified 2 blocks");
-note_wal_summary_dir("after pg_walsummary run", $node1);
 
 done_testing();
-
-# XXX. Temporary debugging code.
-sub note_wal_summary_dir
-{
-	my ($flair, $node) = @_;
-
-	my $wsdir = sprintf "%s/pg_wal/summaries", $node->data_dir;
-	my @wsfiles = grep { $_ ne '.' && $_ ne '..' } slurp_dir($wsdir);
-	note("$flair pg_wal/summaries has: @wsfiles");
-}
-- 
2.25.1

>From 38dda89ee48736489d37e18dea186e90358468b0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Thu, 14 Mar 2024 20:46:02 -0500
Subject: [PATCH v1 2/2] Fix possible overflow in MaybeRemoveOldWalSummaries().

---
 src/backend/postmaster/walsummarizer.c | 4 ++--
 src/backend/utils/misc/guc_tables.c    | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

 
 
-- 
2.25.1

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 19 Mar 2024 13:15:02 -0500

> Subject: [PATCH v1 2/2] Fix possible overflow in MaybeRemoveOldWalSummaries().

Assuming there are no objections or feedback, I plan to commit these two
patches within the next couple of days.

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 20 Mar 2024 13:35:27 -0500

Committed.