Date: Thu, 3 Sep 2026 04:08:19 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Zsolt Parragi <zsolt.parragi@percona.com>
Cc: Daniel Gustafsson <daniel@yesql.se>, pgsql-hackers@lists.postgresql.org
Subject: Re: Offline data checksum changes can cause incorrect checksum state
 on standbys
Message-ID: <apjysxMee6zvSCnk@bdtpg>
References: 
 <CAN4CZFNqKg9Ts76r922cKWcOgtH32ocyghQa8NLB-qeCeC1RKg@mail.gmail.com>
 <apVnwOuJBb4rHk+B@bdtpg>
 <CAN4CZFMFcgfgJ99RYhax-T+YJH=CWLy6ZGANMxf77SrSirw3VQ@mail.gmail.com>
 <apWdicFsT+iFxgAH@bdtpg>
 <CAN4CZFN5sOQvWTXr7Dm1J1HXwczikBfMjkbFQ7QmHsr+VHd5Mg@mail.gmail.com>
 <apaPDmrlhtXgKR+E@bdtpg>
 <CAN4CZFNdfb-yFRW7Sh3FkZ3Qc91Lz-JDQN0b-070-6G35g5Ssg@mail.gmail.com>
 <apfUBpSn9Rj1p+f1@bdtpg>
 <CAN4CZFOOHZmVnL-B2D+V7ODmVFgkYucgQD+-qegDTKZgJ3dtQg@mail.gmail.com>
 <CAN4CZFP_-vMOtHEj0u_v9OYoihxmi1QkfP_wiP_ytdg9FE=KfA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAN4CZFP_-vMOtHEj0u_v9OYoihxmi1QkfP_wiP_ytdg9FE=KfA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/apjysxMee6zvSCnk%40bdtpg>
Precedence: bulk

Hi,

On Wed, Sep 02, 2026 at 03:34:40PM +0100, Zsolt Parragi wrote:
> v11 adds a few more edits based on Daniel's feedback.

So, I compared v9 and v11, and the additional C changes look good to me (persisting
the complete checksum state at the end of recovery and protecting the control file
read with ControlFileLock).

I just have a few wording comments on 0001:

=== 1

+ * record over it.  The control file also carries a watermark, the end LSN of
+ * the newest XLOG2_CHECKSUMS record this node has written or applied.

After pg_rewind, the watermark can be the divergence point rather than the end
of an XLOG2_CHECKSUMS record. Maybe use the wording from pg_control.h here?

=== 2

+     each node in the replication setup.  Nodes can be processed in
+     parallel as they are shut down.  Processing must have ended

s/as they are shut down/while they are shut down/?
s/must have ended/must complete/?

And also, one that was already in v9:

=== 3

+ * record: without that, a checkpoint could read the old state after the
+ * record is already in WAL and insert a redo record that both precedes the
+ * transition in WAL order and carries the pre-transition state.

I think that should be s/precedes/follows/. This would also match the wording in
CreateCheckPoint().

In the commit message:

=== 4

"
Recovery from a base backup is the exception to not adopting
"

I think this is not true for every base backup. A backup taken from a standby
keeps the copied control file state. Otherwise, adoption only happens when the
state is not node local and its watermark is below the starting checkpoint.

Maybe s/is the exception/may be an exception/, with a short mention of those
conditions?

Other than that, v11-0001 looks good to me.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com