Hi all,
On Wed, Jul 15, 2026 at 5:39 PM cca5507 <[email protected]> wrote: > > Hi, > > Thanks for the comments! > > > 1. Commit message understates the fix. It only describes the page-header > > symptom. The crash-safety property is the stronger argument and the one > > that resolves the > back-and-forth on which LSN to use. Suggest adding: > > a. "Using the flush LSN is also crash-safe with respect to the source: the > > insert LSN lives only in shared memory and can be lost on a source crash, > > leaving the standby's minRecoveryPoint ahead of any LSN the source can > > subsequently reach." > > 2. Code comment should explain why flush LSN is sufficient. The current "We > > must replay to the last WAL flush location" doesn't say why. Suggest: > > a. "Use the source's flush LSN as the target's minRecoveryPoint: every > > WAL-logged page we copied has page-LSN <= source's flush LSN at copy time > > (WAL-before-data), and flush LSN is monotonic. We avoid the insert LSN > > because it can sit one page-header past a record's end at segment > > boundaries (where no record will end), and it is not durable, a source > > crash can leave flush LSN behind an insert LSN we already pinned." > > 3. Worth a comment in rewind_source.h that the callback must only be > > invoked against a non-standby source, pg_current_wal_flush_lsn() errors out > > under recovery. > > Fixed. > > > 4. No regression test. We can add a regression test under > > src/bin/pg_rewind/t/. > > Currently I don't have a good idea about the test, I will work on it later. > Any help is welcome! > I reviewed and tested this patch on my PostgreSQL 20devel tree. The patch applied cleanly and built successfully without any issues. I first reproduced the reported issue on an unpatched tree using a two-node streaming replication setup. After promoting the standby, stopping the old primary, ensuring the WAL insert LSN reached the required boundary, and running pg_rewind, the rewound server entered standby mode and started streaming from the promoted primary. However, it remained in recovery but the client connections failed with: psql: error: connection to server on socket "/tmp/.s.PGSQL.55450" failed: FATAL: the database system is not yet accepting connections DETAIL: Consistent recovery state has not been yet reached. Before generating new WAL: LOG: redo starts at 0/07000028 LOG: started streaming WAL from primary at 0/0C000000 on timeline 2 FATAL: the database system is not yet accepting connections DETAIL: Consistent recovery state has not been yet reached. After generating WAL on the promoted primary, the server reached a consistent recovery state only after generating additional WAL on the promoted primary, which matches the behavior described in the discussion thread: LOG: completed backup recovery with redo LSN 0/07000028 and end LSN 0/0C000028 LOG: consistent recovery state reached at 0/0C000048 LOG: database system is ready to accept read-only connections I then applied the V3 patch and repeated the same test. After pg_rewind, the rewound standby completed backup recovery, reached a consistent recovery state, and became ready to accept read-only connections without requiring any additional WAL generation on the promoted primary: LOG: completed backup recovery with redo LSN 0/07000060 and end LSN 0/0E000060 LOG: consistent recovery state reached at 0/0E000060 LOG: invalid record length at 0/0E000060: expected at least 24, got 0 LOG: database system is ready to accept read-only connections LOG: started streaming WAL from primary at 0/0E000000 on timeline 2 I also verified that the rewound server remained in recovery and that the replicated data was accessible. Overall based on my testing, the patch resolves the reported issue, and did not observe any regressions during the tested scenario. The patch looks good to me. Regards, Solai
