From 95cd5a72eb3adbaaabd58761115f4fb2129b8ac1 Mon Sep 17 00:00:00 2001 From: Srinath Reddy Sadipiralla Date: Sat, 29 Aug 2026 11:27:06 +0530 Subject: [PATCH 1/1] Fix pg_rewind file sync bypass and findLastCheckpoint boundary crash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a target cluster is cleanly shut down and a standby is promoted, the target's WAL ends exactly at the divergence point. In this scenario (target_wal_endrec == divergerec), pg_rewind previously assumed no rewind was needed and exited early. While there are zero modified data  blocks to extract, skipping the file sync phase leaves the target missing timeline history files, non-WAL-logged configuration changes  (e.g., postgresql.auto.conf), and a properly updated pg_control file. Removing this check exposes another bug: findLastCheckpoint() crashes with "invalid record length (expected  at least 24, got 0)" when attempting to scan backward. This occurs because divergerec points to the exact end of the target's WAL, but the backward scanner expects a valid record header to start at that exact LSN. This patch fixes both issues: 1. In pg_rewind.c, the target_wal_endrec == divergerec early exit is   removed. If timelines differ, file-level sync is strictly required. 2. In findLastCheckpoint(), if the initial backward read fails exactly   at the forkptr due to unwritten WAL, it gracefully falls back to a   forward scan starting from the pg_control checkpoint LSN to locate   the last common checkpoint. A TAP test is included to demonstrate the file-sync failure on master using an ALTER SYSTEM command immediately after promotion. --- src/bin/pg_rewind/parsexlog.c | 168 +++++++++++++++------- src/bin/pg_rewind/pg_rewind.c | 24 ++-- src/bin/pg_rewind/pg_rewind.h | 2 +- src/bin/pg_rewind/t/012_equal_lsn_sync.pl | 76 ++++++++++ 4 files changed, 207 insertions(+), 63 deletions(-) create mode 100644 src/bin/pg_rewind/t/012_equal_lsn_sync.pl diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c index 023e23b063c..cb578d2a44b 100644 --- a/src/bin/pg_rewind/parsexlog.c +++ b/src/bin/pg_rewind/parsexlog.c @@ -167,9 +167,9 @@ readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex, void findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex, XLogRecPtr *lastchkptrec, TimeLineID *lastchkpttli, - XLogRecPtr *lastchkptredo, const char *restoreCommand) + XLogRecPtr *lastchkptredo, const char *restoreCommand, + XLogRecPtr cntrlfilechkptrec) { - /* Walk backwards, starting from the given record */ XLogRecord *record; XLogRecPtr searchptr; XLogReaderState *xlogreader; @@ -177,6 +177,7 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex, XLogPageReadPrivate private; XLogSegNo current_segno = 0; TimeLineID current_tli = 0; + bool fallback_to_forward = false; /* * The given fork pointer points to the end of the last common record, @@ -200,66 +201,137 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex, if (xlogreader == NULL) pg_fatal("out of memory while allocating a WAL reading processor"); + /* + * Attempt the standard backward scan first. + */ searchptr = forkptr; - for (;;) - { - uint8 info; + XLogBeginRead(xlogreader, searchptr); + record = XLogReadRecord(xlogreader, &errormsg); - XLogBeginRead(xlogreader, searchptr); - record = XLogReadRecord(xlogreader, &errormsg); + if (record == NULL) + { + /* + * If we fail to read exactly at the forkptr, we assume the target crashed + * exactly at a record boundary and the WAL is padded with zeroes. + * Instead of crashing pg_rewind, we fallback to a forward scan. + */ + fallback_to_forward = true; + } - if (record == NULL) + if (!fallback_to_forward) + { + /* We successfully read the first record; proceed with backward scan */ + for (;;) { - if (errormsg) - pg_fatal("could not find previous WAL record at %X/%08X: %s", - LSN_FORMAT_ARGS(searchptr), - errormsg); - else - pg_fatal("could not find previous WAL record at %X/%08X", - LSN_FORMAT_ARGS(searchptr)); - } + uint8 info; - /* Detect if a new WAL file has been opened */ - if (xlogreader->seg.ws_tli != current_tli || - xlogreader->seg.ws_segno != current_segno) - { - char xlogfname[MAXFNAMELEN]; + if (record == NULL) + { + /* A failure mid-scan means real corruption, so we error out */ + if (errormsg) + pg_fatal("could not find previous WAL record at %X/%08X: %s", + LSN_FORMAT_ARGS(searchptr), errormsg); + else + pg_fatal("could not find previous WAL record at %X/%08X", + LSN_FORMAT_ARGS(searchptr)); + } - snprintf(xlogfname, MAXFNAMELEN, XLOGDIR "/"); + /* Detect if a new WAL file has been opened */ + if (xlogreader->seg.ws_tli != current_tli || + xlogreader->seg.ws_segno != current_segno) + { + char xlogfname[MAXFNAMELEN]; + + snprintf(xlogfname, MAXFNAMELEN, XLOGDIR "/"); + current_tli = xlogreader->seg.ws_tli; + current_segno = xlogreader->seg.ws_segno; + XLogFileName(xlogfname + sizeof(XLOGDIR), + current_tli, current_segno, WalSegSz); + keepwal_add_entry(xlogfname); + } - /* update current values */ - current_tli = xlogreader->seg.ws_tli; - current_segno = xlogreader->seg.ws_segno; + /* Check if it is a valid checkpoint record. */ + info = XLogRecGetInfo(xlogreader) & ~XLR_INFO_MASK; + if (searchptr < forkptr && + XLogRecGetRmid(xlogreader) == RM_XLOG_ID && + (info == XLOG_CHECKPOINT_SHUTDOWN || + info == XLOG_CHECKPOINT_ONLINE)) + { + CheckPoint checkPoint; - XLogFileName(xlogfname + sizeof(XLOGDIR), - current_tli, current_segno, WalSegSz); + memcpy(&checkPoint, XLogRecGetData(xlogreader), sizeof(CheckPoint)); + *lastchkptrec = searchptr; + *lastchkpttli = checkPoint.ThisTimeLineID; + *lastchkptredo = checkPoint.redo; + break; + } - /* Track this filename as one to not remove */ - keepwal_add_entry(xlogfname); + /* Walk backwards to previous record. */ + searchptr = record->xl_prev; + XLogBeginRead(xlogreader, searchptr); + record = XLogReadRecord(xlogreader, &errormsg); } + } + else + { + /* + * Fallback. Scan forward from the control file's last checkpoint. + */ + searchptr = cntrlfilechkptrec; + XLogBeginRead(xlogreader, searchptr); - /* - * Check if it is a checkpoint record. This checkpoint record needs to - * be the latest checkpoint before WAL forked and not the checkpoint - * where the primary has been stopped to be rewound. - */ - info = XLogRecGetInfo(xlogreader) & ~XLR_INFO_MASK; - if (searchptr < forkptr && - XLogRecGetRmid(xlogreader) == RM_XLOG_ID && - (info == XLOG_CHECKPOINT_SHUTDOWN || - info == XLOG_CHECKPOINT_ONLINE)) + for (;;) { - CheckPoint checkPoint; + uint8 info; - memcpy(&checkPoint, XLogRecGetData(xlogreader), sizeof(CheckPoint)); - *lastchkptrec = searchptr; - *lastchkpttli = checkPoint.ThisTimeLineID; - *lastchkptredo = checkPoint.redo; - break; - } + record = XLogReadRecord(xlogreader, &errormsg); + + if (record == NULL) + { + if (errormsg) + pg_fatal("could not read WAL record during forward scan at %X/%08X: %s", + LSN_FORMAT_ARGS(searchptr), errormsg); + else + pg_fatal("could not read WAL record during forward scan at %X/%08X", + LSN_FORMAT_ARGS(searchptr)); + } + + /* Update searchptr to the start of the record we just read */ + searchptr = xlogreader->ReadRecPtr; + + /* Detect if a new WAL file has been opened */ + if (xlogreader->seg.ws_tli != current_tli || + xlogreader->seg.ws_segno != current_segno) + { + char xlogfname[MAXFNAMELEN]; + + snprintf(xlogfname, MAXFNAMELEN, XLOGDIR "/"); + current_tli = xlogreader->seg.ws_tli; + current_segno = xlogreader->seg.ws_segno; + XLogFileName(xlogfname + sizeof(XLOGDIR), + current_tli, current_segno, WalSegSz); + keepwal_add_entry(xlogfname); + } - /* Walk backwards to previous record. */ - searchptr = record->xl_prev; + /* Check if it is a checkpoint record. Update pointers iteratively. */ + info = XLogRecGetInfo(xlogreader) & ~XLR_INFO_MASK; + if (searchptr < forkptr && + XLogRecGetRmid(xlogreader) == RM_XLOG_ID && + (info == XLOG_CHECKPOINT_SHUTDOWN || + info == XLOG_CHECKPOINT_ONLINE)) + { + CheckPoint checkPoint; + + memcpy(&checkPoint, XLogRecGetData(xlogreader), sizeof(CheckPoint)); + *lastchkptrec = searchptr; + *lastchkpttli = checkPoint.ThisTimeLineID; + *lastchkptredo = checkPoint.redo; + } + + /* If we've reached or passed the divergence point, we are done */ + if (xlogreader->EndRecPtr >= forkptr) + break; + } } XLogReaderFree(xlogreader); diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c index 2e86fd158d0..fdeced703cb 100644 --- a/src/bin/pg_rewind/pg_rewind.c +++ b/src/bin/pg_rewind/pg_rewind.c @@ -439,22 +439,18 @@ main(int argc, char **argv) target_wal_endrec = chkptendrec; } + pg_log_info("target WAL ends at %X/%08X", LSN_FORMAT_ARGS(target_wal_endrec)); + /* - * Check for the possibility that the target is in fact a direct - * ancestor of the source. In that case, there is no divergent history - * in the target that needs rewinding. + * The target WAL cannot end before the divergence point. + * If target_wal_endrec == divergerec, the target wrote no WAL past the + * fork point. However, because the timelines diverged, we must still + * synchronize non-WAL files and fetch the new timeline history. */ - if (target_wal_endrec > divergerec) - { - rewind_needed = true; - } - else - { - /* the last common checkpoint record must be part of target WAL */ - Assert(target_wal_endrec == divergerec); + if (target_wal_endrec < divergerec) + pg_fatal("target WAL ends before the divergence point"); - rewind_needed = false; - } + rewind_needed = true; } if (!rewind_needed) @@ -471,7 +467,7 @@ main(int argc, char **argv) keepwal_init(); findLastCheckpoint(datadir_target, divergerec, lastcommontliIndex, - &chkptrec, &chkpttli, &chkptredo, restore_command); + &chkptrec, &chkpttli, &chkptredo, restore_command, ControlFile_target.checkPoint); pg_log_info("rewinding from last common checkpoint at %X/%08X on timeline %u", LSN_FORMAT_ARGS(chkptrec), chkpttli); diff --git a/src/bin/pg_rewind/pg_rewind.h b/src/bin/pg_rewind/pg_rewind.h index 9a981f7f246..bc3f700a76e 100644 --- a/src/bin/pg_rewind/pg_rewind.h +++ b/src/bin/pg_rewind/pg_rewind.h @@ -39,7 +39,7 @@ extern void findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex, XLogRecPtr *lastchkptrec, TimeLineID *lastchkpttli, XLogRecPtr *lastchkptredo, - const char *restoreCommand); + const char *restoreCommand, XLogRecPtr cntrlfilechkptrec); extern XLogRecPtr readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex, const char *restoreCommand); diff --git a/src/bin/pg_rewind/t/012_equal_lsn_sync.pl b/src/bin/pg_rewind/t/012_equal_lsn_sync.pl new file mode 100644 index 00000000000..906f6ffdb4a --- /dev/null +++ b/src/bin/pg_rewind/t/012_equal_lsn_sync.pl @@ -0,0 +1,76 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +# Initialize and start the primary node (Node A) +my $node_a = PostgreSQL::Test::Cluster->new('node_a'); +$node_a->init(allows_streaming => 1); +$node_a->start; + +# Initialize the standby node (Node B) from a backup of Node A +my $node_b = PostgreSQL::Test::Cluster->new('node_b'); +$node_a->backup('my_backup'); +$node_b->init_from_backup($node_a, 'my_backup', has_streaming => 1); +$node_b->start; + +# Wait for the standby to catch up to ensure WAL is identical +$node_a->wait_for_catchup($node_b, 'replay', $node_a->lsn('insert')); + +# stop NODE A cleanly. +# This is the critical step to trigger the bug. A clean shutdown writes a +# shutdown checkpoint. Node A's WAL now ends exactly at this LSN. +$node_a->stop; + +# Promote Node B. +# It writes an end-of-recovery record at the exact LSN where Node A stopped. +# divergerec will now perfectly equal target_wal_endrec. +$node_b->promote; + +# Make a non-WAL-logged change on the new primary (Node B). +# ALTER SYSTEM modifies postgresql.auto.conf but generates no WAL. +$node_b->safe_psql('postgres', "ALTER SYSTEM SET work_mem = '50MB';"); + +# Run pg_rewind using run_command to capture all output +my ($stdout, $stderr) = run_command( + [ + 'pg_rewind', '--debug', + '--source-server' => $node_b->connstr, + '--target-pgdata' => $node_a->data_dir, + '--no-sync' + ] +); + +my ($divergerec) = $stderr =~ /servers diverged at WAL location ([A-F0-9X\/]+) on timeline/; +my ($target_wal_end) = $stderr =~ /target WAL ends at ([A-F0-9X\/]+)/; + +ok(defined $divergerec, "Found divergence LSN in logs: $divergerec"); +ok(defined $target_wal_end, "Found target WAL end LSN in logs: $target_wal_end"); + +# It asserts that the test successfully triggered the exact boundary condition. +is( + $target_wal_end, + $divergerec, + 'Target WAL ends exactly at the divergence point (0 WAL changes)' +); + +# If pg_rewind wrongly assumes it can skip the sync, it prints this exact line. +# We want to ensure it DOES NOT print this line. +unlike( + $stderr, + qr/no rewind required/, + 'pg_rewind correctly recognized that differing timelines require a rewind' +); + +# Verify the file sync actually occurred +my $auto_conf_target = slurp_file($node_a->data_dir . '/postgresql.auto.conf'); +like( + $auto_conf_target, + qr/work_mem = '50MB'/, + 'postgresql.auto.conf was synchronized from the source even WAL LSNs matched' +); + +done_testing(); \ No newline at end of file -- 2.43.0