Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: pg_waldump: support decoding of WAL inside tarfile
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAAJ_b94f6sUDWiZY90O-t7SWWeSK0nMWT7AsydKkpAL90m0oMQ@mail.gmail.com>
Date: Tue, 25 Nov 2025 16:50:29 +0800
Cc: Jakub Wartak <jakub.wartak@enterprisedb.com>,
 Robert Haas <robertmhaas@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <731ADE6F-01C5-4996-BAEE-5851DFC3F502@gmail.com>
References: 
 <CAAJ_b94bqdWN3h2J-PzzzQ2Npbwct5ZQHggn_QoYGhC2rn-=WQ@mail.gmail.com>
 <CAAJ_b97PQjE4kFD8Qk6UvtLrfPMixw1nxBz0OP5Z2WB2B-uMxQ@mail.gmail.com>
 <CAAJ_b97JA8ehy_UDddrnGwDt9HG5NmJq8ATtmeMqo7YD-=tLyQ@mail.gmail.com>
 <CA+TgmoZjhWDG_AR1i+L1yss-wbuWvxrdRwSdVUUUnVPrJV2CnQ@mail.gmail.com>
 <CAAJ_b94Uh+b41LQG45bZFK+i62EVvv972LiGWWWuR64=-64rTQ@mail.gmail.com>
 <CA+TgmobF5c7ZcZHdEhqwNxGDZzWG2bDtpRaDtoVELWX_VHs_1A@mail.gmail.com>
 <CAAJ_b94gK1np8d1h-2c1YoCccGXr4zspTa-FC7X_bfXZNz=-DA@mail.gmail.com>
 <CA+TgmoayDY5b+bP1vRRN7A3xOP-=+tK13B2C1g-Xm1j4WTrT9Q@mail.gmail.com>
 <CAAJ_b97JAF+Zuoh2FBO79hVwLeaBPwsbXw-fY+313a7LfRQ-Bg@mail.gmail.com>
 <CA+Tgmoardk4VuthHc23vov+AVkhq7eT0mFUs-2ctAnP1uiTaog@mail.gmail.com>
 <CAAJ_b959x5VjmLJFmN78r_QohQuuj=fde11VbbAOHn5TzgEzng@mail.gmail.com>
 <CAAJ_b97_N+0sipFyq80n0jX-nKcjcQEMOSTVg8DsqkHR8dW_Sw@mail.gmail.com>
 <CAKZiRmyDk5KqovS9Ez3iFHd+p-TChSt2QTtWkwJ5Ya-+4gg21g@mail.gmail.com>
 <CAAJ_b956a+e8-HNEpeJ60ByFv7XJRqECPu3B0dozv0ChMRTPbQ@mail.gmail.com>
 <CAAJ_b94f6sUDWiZY90O-t7SWWeSK0nMWT7AsydKkpAL90m0oMQ@mail.gmail.com>
To: Amul Sul <sulamul@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/731ADE6F-01C5-4996-BAEE-5851DFC3F502%40gmail.com>
Precedence: bulk

Hi Amul,

I reviewed the patch and got some comments:

> On Nov 25, 2025, at 14:37, Amul Sul <sulamul@gmail.com> wrote:
>=20
>=20
> Regards,
> Amul
> =
<v8-0001-Refactor-pg_waldump-Move-some-declarations-to-new.patch><v8-0002-=
Refactor-pg_waldump-Separate-logic-used-to-calcul.patch><v8-0003-Refactor-=
pg_waldump-Restructure-TAP-tests.patch><v8-0004-pg_waldump-Add-support-for=
-archived-WAL-decoding.patch><v8-0005-pg_waldump-Remove-the-restriction-on=
-the-order-of.patch><v8-0006-pg_verifybackup-Delay-default-WAL-directory-p=
repa.patch><v8-0007-pg_verifybackup-Rename-the-wal-directory-switch-t.patc=
h><v8-0008-pg_verifybackup-enabled-WAL-parsing-for-tar-forma.patch>

1 - 0001 - pg_waldump.h
```
+ * pg_waldump.h - decode and display WAL
+ *
+ * Copyright (c) 2013-2025, PostgreSQL Global Development Group
```

This header file is brand new, so copyright year should be only 2025.

2 - 0001 - pg_waldump.c
```
-static int	WalSegSz;
+int			WalSegSz =3D DEFAULT_XLOG_SEG_SIZE;
```

0001 claims a refactoring, but if you initialize WalSegSz with =
DEFAULT_XLOG_SEG_SIZE, then the behavior is changing, this change is no =
longer a pure refactor.

I would suggest leave WalSegSz uninitiated (compiler will set 0 to it), =
then no behavior change, so that 0001 stays a self-contained pure =
refactor.

The other nit thing is that, as =E2=80=9Cstatic=E2=80=9D is removed, now =
=E2=80=9CWalSegSz=E2=80=9D is placed in middle of two static variables, =
which looks not good. If I were making the code change, I would have =
moved WalSegSz to after all static variables.

3 - 0002
```
@@ -383,21 +406,11 @@ WALDumpReadPage(XLogReaderState *state, XLogRecPtr =
targetPagePtr, int reqLen,
 				XLogRecPtr targetPtr, char *readBuff)
 {
 	XLogDumpPrivate *private =3D state->private_data;
-	int			count =3D XLOG_BLCKSZ;
+	int			count =3D required_read_len(private, =
targetPagePtr, reqLen);
 	WALReadError errinfo;
=20
-	if (XLogRecPtrIsValid(private->endptr))
-	{
-		if (targetPagePtr + XLOG_BLCKSZ <=3D private->endptr)
-			count =3D XLOG_BLCKSZ;
-		else if (targetPagePtr + reqLen <=3D private->endptr)
-			count =3D private->endptr - targetPagePtr;
-		else
-		{
-			private->endptr_reached =3D true;
-			return -1;
-		}
-	}
+	if (private->endptr_reached)
+		return -1;
```

This change introduces a logic hole. In old code, it sets =
private->endptr_reached =3D true; and return -1. In the code code, count =
and private->endptr_reached assignments are wrapped into =
required_read_len(). However, required_read_len() doesn=E2=80=99t check =
if private->endptr_reached has already been true, so that the logic hole =
is that, if private->endptr_reached is already true when calling =
required_read_len(), and required_read_len() returns a positive count, =
if (private->endptr_reached) will also be satisfied and return -1 from =
the function.

So, to be safe, we should check =E2=80=9Cif (count < 0) return -1=E2=80=9D=
.

4 - 0002
```
+/* Returns the size in bytes of the data to be read. */
+static inline int
+required_read_len(XLogDumpPrivate *private, XLogRecPtr targetPagePtr,
+				  int reqLen)
+{
```

The function comment is too simple. It doesn=E2=80=99t cover the case =
where -1 is returned.

5 - 0003
```
+my @scenario =3D (
+	{
+		'path' =3D> $node->data_dir
+	});
=20
-@lines =3D test_pg_waldump('--limit' =3D> 6);
-is(@lines, 6, 'limit option observed');
+for my $scenario (@scenario)
+{
```

"my @scenario=E2=80=9D should be "my @scenarios=E2=80=9D, so that for =
line become "for my $scenario (@scenarios)=E2=80=9D, a little bit =
clearer.

6 - 0003
```
+	SKIP:
+	{
```

Why SKIP label is defined here? A SKIP label usually follows a skip =
statement, for example: in bin/pg_ctl/t/001_start_stop.pl
```
SKIP:
{
skip "unix-style permissions not supported on Windows", 2
if ($windows_os);

ok(-f $logFileName);
ok(check_mode_recursive("$tempdir/data", 0700, 0600));
}
```

7 - 0004 - Makefile
```
 	$(WIN32RES) \
 	compat.o \
 	pg_waldump.o \
+	archive_waldump.o \
 	rmgrdesc.o \
 	xlogreader.o \
 	xlogstats.o
```

Obviously the list was in alphabetical order, so archive_waldump.o =
should be placed before compat.o.

8 - 0004
```
+/*
+ * pg_waldump's XLogReaderRoutine->page_read callback to support =
dumping WAL
+ * files from tar archives.
+ */
+static int
+TarWALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, =
int reqLen,
+				   XLogRecPtr targetPtr, char *readBuff)
+{
+	XLogDumpPrivate *private =3D state->private_data;
+	int			count =3D required_read_len(private, =
targetPagePtr, reqLen);
```

Looking the page_read=E2=80=99s spec:
```
	/*
	 * Data input callback
	 *
	 * This callback shall read at least reqLen valid bytes of the =
xlog page
	 * starting at targetPagePtr, and store them in readBuf.  The =
callback
	 * shall return the number of bytes read (never more than =
XLOG_BLCKSZ), or
	 * -1 on failure.  The callback shall sleep, if necessary, to =
wait for the
	 * requested bytes to become available.  The callback will not =
be invoked
	 * again for the same page unless more than the returned number =
of bytes
	 * are needed.
	 *
	 * targetRecPtr is the position of the WAL record we're reading. =
 Usually
	 * it is equal to targetPagePtr + reqLen, but sometimes =
xlogreader needs
	 * to read and verify the page or segment header, before it =
reads the
	 * actual WAL record it's interested in.  In that case, =
targetRecPtr can
	 * be used to determine which timeline to read the page from.
	 *
	 * The callback shall set ->seg.ws_tli to the TLI of the file =
the page was
	 * read from.
	 */
	XLogPageReadCB page_read;
```

It says that page_read must read reqLen bytes, otherwise it should wait =
for more bytes.

However, TarWALDumpReadPage just calculate how many bytes can read and =
only read that long, which breaks the protocol. Is it a problem?

9 - 0004
```
+/*
+ * Create an astreamer that can read WAL from tar file.
+ */
+static astreamer *
+astreamer_waldump_new(XLogDumpPrivate *privateInfo)
+{
+	astreamer_waldump *streamer;
+
+	streamer =3D palloc0(sizeof(astreamer_waldump));
+	*((const astreamer_ops **) &streamer->base.bbs_ops) =3D
+		&astreamer_waldump_ops;
+
+	streamer->privateInfo =3D privateInfo;
+
+	return &streamer->base;
+}
```

This function allocates memory for streamer but only returns =
&streamer->base, so memory of streamer is leaked.

Also, in the function comment, =E2=80=9Cfrom tar file=E2=80=9D =3D> =
=E2=80=9Cfrom a tar file=E2=80=9D.

10 - 0004
```
+ * End-of-stream processing for a astreamer_waldump stream.
```

Nit typo: a =3D> an

11 - 0004
```
+	if (!IsValidWalSegSize(WalSegSz))
+	{
+		pg_log_error(ngettext("invalid WAL segment size in WAL =
file from archive \"%s\" (%d byte)",
+							  "invalid WAL =
segment size in WAL file from archive \"%s\" (%d bytes)",
+							  WalSegSz),
+					 privateInfo->archive_name, =
WalSegSz);
+		pg_log_error_detail("The WAL segment size must be a =
power of two between 1 MB and 1 GB.");
+		exit(1);
+	}
```

Why don=E2=80=99t pg_fatal()?

12 - 0005
```
+		/* Create a temporary file if one does not already exist =
*/
+		if (!entry->tmpseg_exists)
+		{
+			write_fp =3D prepare_tmp_write(entry->segno);
+			entry->tmpseg_exists =3D true;
+		}
+
+		/* Flush data from the buffer to the file */
+		perform_tmp_write(entry->segno, &entry->buf, write_fp);
+		resetStringInfo(&entry->buf);
+
+		/*
+		 * The change in the current segment entry indicates =
that the reading
+		 * of this file has ended.
+		 */
+		if (entry !=3D privateInfo->cur_wal && write_fp !=3D =
NULL)
+		{
+			fclose(write_fp);
+			write_fp =3D NULL;
+		}
```

When entry->tmpseg_exists is true, then write_fp will not be =
initialized, but there should be a check to make sure write_fp is not =
NULL before perform_tmp_write().=20

Also, if write_fp !=3D NULL, should we anyway close the file without =
considering entry !=3D privateInfo->cur_wal? Otherwise write_fp may be =
left open.

13 - 0005
```
+	 * Use the directory specified by the TEMDIR environment =
variable. If it=E2=80=99s
```

Typo: TEMDIR =3D> TMPDIR

14 - 0005
```
+ * Set up a temporary directory to temporarily store WAL segments.
```

temporary and temporarily are redundant.

No comment for 0007.

15 - 0007

I wonder why we need to manually po files? This is the first time I see =
a patch including po file changes.

16 - 0008
```
+		{
+			pg_log_error("wal archive not found");
+			pg_log_error_hint("Specify the correct path =
using the option -w/--wal-path."
+							  "Or you must =
use -n/--no-parse-wal when verifying a tar-format backup.");
+			exit(1);
+		}
```

=E2=80=9Cwal=E2=80=9D should be =E2=80=9CWAL=E2=80=9D.

In the hint message, there should be a white space between the two =
sentences.

Again, why not pg_fatal().

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/