From: Haibo Yan <tristan.yim@gmail.com>
Message-Id: <7F7B289B-F94F-42C2-9E54-6A689C0D64BB@gmail.com>
Content-Type: multipart/alternative;
	boundary="Apple-Mail=_5FEC83E2-5B7D-4582-9E25-9706B8E1489E"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: Return pg_control from pg_backup_stop().
Date: Tue, 17 Mar 2026 11:50:29 -0700
In-Reply-To: <feb68313-3257-4d9d-8b69-6468e506a61a@pgbackrest.org>
Cc: Michael Paquier <michael@paquier.xyz>,
 Pg Hackers <pgsql-hackers@postgresql.org>
To: David Steele <david@pgbackrest.org>
References: <e2636c5d-c031-43c9-a5d6-5e5c7e4c5514@pgmasters.net>
 <ZvzyL0xPUa06_O9B@paquier.xyz>
 <83f5295e-082d-432c-92a4-365707bd2296@pgbackrest.org>
 <Zv4hdzZURFAYYAL5@paquier.xyz>
 <1248c005-b693-494a-8d7a-68bc7d482679@pgbackrest.org>
 <fe9a5423-c36d-43ad-9884-dee4c3b7dc6d@pgbackrest.org>
 <edc09339-3a4c-4195-8c06-7aa2953ca5be@pgbackrest.org>
 <d388dba5-dc68-4dab-9b8d-ca301956e85b@pgbackrest.org>
 <8b8aa673-fcef-4e14-a05d-0885283ef1b8@pgbackrest.org>
 <d21f3c7c-563c-49db-8983-faa73ed4369f@pgbackrest.org>
 <f74cab02-5ebb-439c-975b-29d019eed787@pgbackrest.org>
 <17DC1346-0CDE-4E39-B110-3D6FB0797AC6@gmail.com>
 <feb68313-3257-4d9d-8b69-6468e506a61a@pgbackrest.org>
Archived-At: 
 <https://www.postgresql.org/message-id/7F7B289B-F94F-42C2-9E54-6A689C0D64BB%40gmail.com>
Precedence: bulk


--Apple-Mail=_5FEC83E2-5B7D-4582-9E25-9706B8E1489E
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

Hi David,

Thank you for the clarification. I have now read the code, and overall =
it looks good to me. I only had one very small comment.

You currently have:
```
memset(controlFile + sizeof(ControlFileData), 0,
       PG_CONTROL_FILE_SIZE - sizeof(ControlFileData));
memcpy(controlFile, ControlFile, sizeof(ControlFileData));
```=20
This is correct, since only the trailing bytes need to be zeroed before =
the copy.

I was just wondering whether the following might be slightly clearer:
```
memset(controlFile, 0, PG_CONTROL_FILE_SIZE);
memcpy(controlFile, ControlFile, sizeof(ControlFileData));
```

I do not think this is a real issue, though.

Thanks
Haibo

> On Mar 17, 2026, at 12:05=E2=80=AFAM, David Steele =
<david@pgbackrest.org> wrote:
>=20
> On 3/17/26 12:16, Haibo Yan wrote:
>=20
>> I have not read the code yet, so this may already be answered there, =
but I had a question about the proposal itself. This patch protects =
against a missing backup_label, but what about a wrong one? If a user =
restores a backup_label file from a different backup, the existence =
check alone would not detect that. Do we need some consistency check =
between the returned pg_control copy and the backup_label contents, or =
is the intended scope here limited to the =E2=80=9Cmissing file=E2=80=9D =
case only?
>=20
> Thank you for having a look!
>=20
> The goal here is only to check for a missing backup_label. The general =
problem is that PostgreSQL suggests that removing backup_label might be =
a good idea so the user does it:
>=20
> If you are not restoring from a backup, try removing the file =
\"%s/backup_label\"
>=20
> The user *could* copy a backup_label from another backup and there are =
ways we could detect that but I feel that should be material for a =
separate patch.
>=20
> Regards,
> -David
>=20
>=20


--Apple-Mail=_5FEC83E2-5B7D-4582-9E25-9706B8E1489E
Content-Transfer-Encoding: quoted-printable
Content-Type: text/html;
	charset=utf-8

<html aria-label=3D"message body"><head><meta http-equiv=3D"content-type" =
content=3D"text/html; charset=3Dutf-8"></head><body =
style=3D"overflow-wrap: break-word; -webkit-nbsp-mode: space; =
line-break: after-white-space;"><div><font face=3D"Menlo">Hi =
David,</font></div><div><font face=3D"Menlo"><br></font></div><div><font =
face=3D"Menlo">Thank you for the clarification. I have now read the =
code, and overall it looks good to me. I only had one very small =
comment.</font></div><div><font face=3D"Menlo"><br></font></div><div><font=
 face=3D"Menlo">You currently have:</font></div><div><font =
face=3D"Menlo">```</font></div><div><font =
face=3D"Menlo">memset(controlFile + sizeof(ControlFileData), =
0,</font></div><div><font face=3D"Menlo">&nbsp; &nbsp; &nbsp; =
&nbsp;PG_CONTROL_FILE_SIZE - =
sizeof(ControlFileData));</font></div><div><font =
face=3D"Menlo">memcpy(controlFile, ControlFile, =
sizeof(ControlFileData));</font></div><div><font =
face=3D"Menlo">```&nbsp;</font></div><div><font face=3D"Menlo">This is =
correct, since only the trailing bytes need to be zeroed before the =
copy.</font></div><div><font face=3D"Menlo"><br></font></div><div><font =
face=3D"Menlo">I was just wondering whether the following might be =
slightly clearer:</font></div><div><font =
face=3D"Menlo">```</font></div><div><font =
face=3D"Menlo">memset(controlFile, 0, =
PG_CONTROL_FILE_SIZE);</font></div><div><font =
face=3D"Menlo">memcpy(controlFile, ControlFile, =
sizeof(ControlFileData));</font></div><div><font =
face=3D"Menlo">```</font></div><div><font =
face=3D"Menlo"><br></font></div><div><font face=3D"Menlo">I do not think =
this is a real issue, though.</font></div><div><font =
face=3D"Menlo"><br></font></div><div><font =
face=3D"Menlo">Thanks</font></div><div><font =
face=3D"Menlo">Haibo</font></div><div><br><blockquote =
type=3D"cite"><div>On Mar 17, 2026, at 12:05=E2=80=AFAM, David Steele =
&lt;david@pgbackrest.org&gt; wrote:</div><br =
class=3D"Apple-interchange-newline"><div><div>On 3/17/26 12:16, Haibo =
Yan wrote:<br><br><blockquote type=3D"cite">I have not read the code =
yet, so this may already be answered there, but I had a question about =
the proposal itself. This patch protects against a missing backup_label, =
but what about a wrong one? If a user restores a backup_label file from =
a different backup, the existence check alone would not detect that. Do =
we need some consistency check between the returned pg_control copy and =
the backup_label contents, or is the intended scope here limited to the =
=E2=80=9Cmissing file=E2=80=9D case only?<br></blockquote><br>Thank you =
for having a look!<br><br>The goal here is only to check for a missing =
backup_label. The general problem is that PostgreSQL suggests that =
removing backup_label might be a good idea so the user does =
it:<br><br>If you are not restoring from a backup, try removing the file =
\"%s/backup_label\"<br><br>The user *could* copy a backup_label from =
another backup and there are ways we could detect that but I feel that =
should be material for a separate =
patch.<br><br>Regards,<br>-David<br><br><br></div></div></blockquote></div=
><br></body></html>=

--Apple-Mail=_5FEC83E2-5B7D-4582-9E25-9706B8E1489E--