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! -- Regards, ChangAo Chen
v3-0001-pg_rewind-use-flush-lsn-to-set-minRecoveryPoint.patch
Description: Binary data
