Support pg_rewind used without wal_log_hints and data_checksums#1349
Support pg_rewind used without wal_log_hints and data_checksums#1349movead123 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR extends pg_rewind with bidirectional WAL scanning controlled by a new --deep-dig option: it performs a forward page-map extraction, evaluates tracked XID bounds for safety, and when page consistency is not guaranteed can optionally run a backward WAL scan to discover additional modified pages. ChangesBidirectional WAL Scanning with Deep-Dig Support
Sequence DiagramsequenceDiagram
participant Main as pg_rewind main
participant Extract as extractPageMapForward
participant Safety as check_rewind_safety
participant BackExtract as extractPageMapBackward
Main->>Extract: scan forward from target state
Extract-->>Main: page map
alt page_consistence is false
Main->>Safety: check if XID bounds safe
alt unsafe and not deep_dig
Main->>Main: fatal error
else unsafe and deep_dig
Main->>BackExtract: scan backward from checkpoint
BackExtract-->>Main: additional page map
else safe
Main->>Main: proceed
end
else page_consistence is true
Main->>Main: proceed
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.22.0)OpenGrep fatal error (exit code 2): [00.18][ERROR]: Error: exception Unix_error: No such file or directory stat src/bin/pg_rewind/pg_rewind.c Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/bin/pg_rewind/parsexlog.c (4)
49-51: ⚖️ Poor tradeoffPotential re-entrancy hazard with file-scoped globals.
The global XID tracking variables (
min_commited_xid,max_commited_xid,base_xid) are never explicitly reset. While the current call pattern inpg_rewind.censures they're used only once per execution, this design is fragile if the code is ever refactored to allow multiple rewind operations or if these functions are called in a different order.Consider initializing these at the start of
extractPageMapForwardor introducing a reset function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bin/pg_rewind/parsexlog.c` around lines 49 - 51, The file-scoped XID trackers min_commited_xid, max_commited_xid, and base_xid are left set between uses; modify the code to reset them before each rewind run by initializing them at the start of extractPageMapForward (or call a new reset function from the start of extractPageMapForward). Specifically, set min_commited_xid = InvalidTransactionId, max_commited_xid = InvalidTransactionId, and base_xid = InvalidTransactionId at the beginning of extractPageMapForward (or implement and call a static reset_xid_trackers() that does the same) so repeated calls or future refactors won’t retain stale state.
142-148: 💤 Low valueDuplicate page-boundary logic.
This page-header-skipping logic is identical to the code in
findLastCheckpoint(lines 259-265). Consider extracting it into a helper function likeSkipPageHeaderIfNeeded(XLogRecPtr ptr)to avoid maintenance drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bin/pg_rewind/parsexlog.c` around lines 142 - 148, The page-header-skipping logic duplicated around startpoint should be extracted into a single helper (e.g. SkipPageHeaderIfNeeded) so both this site and the identical block in findLastCheckpoint can call it; implement SkipPageHeaderIfNeeded taking an XLogRecPtr (or pointer/ref) and performing the same checks using XLOG_BLCKSZ, XLogSegmentOffset(ptr, WalSegSz), and adding SizeOfXLogLongPHD or SizeOfXLogShortPHD as appropriate, then replace the local duplicated block around the variable startpoint and the block in findLastCheckpoint with calls to that helper.
498-560: 💤 Low valueTracking snapshot in both forward and backward scans.
The code correctly tracks
base_xidfromXLOG_RUNNING_XACTSin both forward and backward scans (lines 544-559 have no backward guard). This is an optimization: if a running-xacts record soon after the fork has anextXidbelow all post-fork commits, the backward scan can be skipped.However, the log message at line 556 says "Get base xid" as if finding a definitive value, when in backward scan multiple progressively earlier snapshots may be found. Consider logging "Found earlier base xid: ..." for clarity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bin/pg_rewind/parsexlog.c` around lines 498 - 560, In track_rewind_snapshot(), the log for updating base_xid (when parsing RM_STANDBY_ID / XLOG_RUNNING_XACTS) should reflect that during backward scans we may find progressively earlier snapshots; modify the pg_log_info call that currently says "Get base xid: %u at %X/%X" to use a different message when backward is true (e.g. "Found earlier base xid: %u at %X/%X") while retaining the original wording for forward scans; locate this in function track_rewind_snapshot() where base_xid is assigned from xlrec->nextXid and conditionally call pg_log_info with the message chosen based on the backward parameter.
678-695: 💤 Low valueDocument function origin.
This function duplicates
TransactionIdPrecedesfrom backendtransam.c(frontend code cannot link backend code). Add a comment to document this, making future maintenance easier:/* * RewindTransactionIdPrecedes --- is id1 logically < id2? * * Adapted from TransactionIdPrecedes in src/backend/access/transam/transam.c * for use in frontend pg_rewind. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bin/pg_rewind/parsexlog.c` around lines 678 - 695, Add a short provenance comment above the RewindTransactionIdPrecedes function documenting that it duplicates/adapts the backend implementation so maintainers know the origin; specifically state it is adapted from TransactionIdPrecedes in the backend transam.c for use in the frontend pg_rewind and include that context in the existing function comment block.src/bin/pg_rewind/pg_rewind.c (1)
107-107: ⚡ Quick winClarify --deep-dig help text.
The help text "perform a deep dig for more wal" is vague. Consider a more descriptive message that explains when this option is needed:
- printf(_(" -d, --deep-dig perform a deep dig for more wal\n")); + printf(_(" -d, --deep-dig enable backward WAL scan when page\n" + " consistency cannot be guaranteed\n"));This helps users understand the option is specifically for cases when neither
wal_log_hintsnor data checksums are enabled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bin/pg_rewind/pg_rewind.c` at line 107, Update the help string printed in pg_rewind.c for the --deep-dig option (the printf call that currently outputs " -d, --deep-dig perform a deep dig for more wal") to a clearer description that states this option forces a more thorough scan of pages to locate WAL when neither wal_log_hints nor data checksums are enabled; mention that it is slower but required in those cases so users know when to use -d/--deep-dig.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bin/pg_rewind/parsexlog.c`:
- Around line 159-182: The function extractPageMapBackward lacks a defensive
guard against an unset min_commited_xid; add an early check at the top of
extractPageMapBackward that tests if min_commited_xid == InvalidTransactionId
and call pg_fatal with a clear message (e.g., "extractPageMapBackward called
without committed transactions") to prevent infinite backward WAL scanning; keep
the existing termination check (base_xid <= min_commited_xid) and the rest of
the loop unchanged.
- Around line 345-354: The safety logic in check_rewind_safety must explicitly
handle the "no snapshot found" case: if min_commited_xid == InvalidTransactionId
return true, then if base_xid == InvalidTransactionId return false (force a
backward scan), otherwise keep the existing comparison (if base_xid <=
min_commited_xid return true; else false); update the check_rewind_safety
function to add the explicit base_xid == InvalidTransactionId branch so an unset
base_xid (left as InvalidTransactionId by extractPageMapForward) does not
incorrectly permit rewinding.
In `@src/bin/pg_rewind/pg_rewind.c`:
- Around line 508-518: Fix the conditional block that handles the backward scan:
adjust style to use spaces after if (e.g., if (!page_consistence) and if
(!deep_dig)), correct grammar and clarity in the pg_fatal message (replace "Can
not safety rewind the target cluster, use --deep-dig to have a try." with a
clearer message such as "Cannot safely rewind the target cluster; rerun with
--deep-dig to attempt a deeper scan."), and leave the call to
extractPageMapBackward(datadir_target, chkptrec, lastcommontliIndex,
restore_command) unchanged when deep_dig is true; retain the
check_rewind_safety() logic and the use of page_consistence and deep_dig flags.
---
Nitpick comments:
In `@src/bin/pg_rewind/parsexlog.c`:
- Around line 49-51: The file-scoped XID trackers min_commited_xid,
max_commited_xid, and base_xid are left set between uses; modify the code to
reset them before each rewind run by initializing them at the start of
extractPageMapForward (or call a new reset function from the start of
extractPageMapForward). Specifically, set min_commited_xid =
InvalidTransactionId, max_commited_xid = InvalidTransactionId, and base_xid =
InvalidTransactionId at the beginning of extractPageMapForward (or implement and
call a static reset_xid_trackers() that does the same) so repeated calls or
future refactors won’t retain stale state.
- Around line 142-148: The page-header-skipping logic duplicated around
startpoint should be extracted into a single helper (e.g.
SkipPageHeaderIfNeeded) so both this site and the identical block in
findLastCheckpoint can call it; implement SkipPageHeaderIfNeeded taking an
XLogRecPtr (or pointer/ref) and performing the same checks using XLOG_BLCKSZ,
XLogSegmentOffset(ptr, WalSegSz), and adding SizeOfXLogLongPHD or
SizeOfXLogShortPHD as appropriate, then replace the local duplicated block
around the variable startpoint and the block in findLastCheckpoint with calls to
that helper.
- Around line 498-560: In track_rewind_snapshot(), the log for updating base_xid
(when parsing RM_STANDBY_ID / XLOG_RUNNING_XACTS) should reflect that during
backward scans we may find progressively earlier snapshots; modify the
pg_log_info call that currently says "Get base xid: %u at %X/%X" to use a
different message when backward is true (e.g. "Found earlier base xid: %u at
%X/%X") while retaining the original wording for forward scans; locate this in
function track_rewind_snapshot() where base_xid is assigned from xlrec->nextXid
and conditionally call pg_log_info with the message chosen based on the backward
parameter.
- Around line 678-695: Add a short provenance comment above the
RewindTransactionIdPrecedes function documenting that it duplicates/adapts the
backend implementation so maintainers know the origin; specifically state it is
adapted from TransactionIdPrecedes in the backend transam.c for use in the
frontend pg_rewind and include that context in the existing function comment
block.
In `@src/bin/pg_rewind/pg_rewind.c`:
- Line 107: Update the help string printed in pg_rewind.c for the --deep-dig
option (the printf call that currently outputs " -d, --deep-dig
perform a deep dig for more wal") to a clearer description that states this
option forces a more thorough scan of pages to locate WAL when neither
wal_log_hints nor data checksums are enabled; mention that it is slower but
required in those cases so users know when to use -d/--deep-dig.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c0526a5-da16-4f0b-b41a-390756b43156
📒 Files selected for processing (6)
src/bin/pg_rewind/.gitignoresrc/bin/pg_rewind/Makefilesrc/bin/pg_rewind/meson.buildsrc/bin/pg_rewind/parsexlog.csrc/bin/pg_rewind/pg_rewind.csrc/bin/pg_rewind/pg_rewind.h
| for (;;) | ||
| { | ||
| XLogBeginRead(xlogreader, searchptr); | ||
| record = XLogReadRecord(xlogreader, &errormsg); | ||
|
|
||
| if (record == NULL) | ||
| { | ||
| if (errormsg) | ||
| pg_fatal("could not find previous WAL record at %X/%X: %s", | ||
| LSN_FORMAT_ARGS(searchptr), | ||
| errormsg); | ||
| else | ||
| pg_fatal("could not find previous WAL record at %X/%X", | ||
| LSN_FORMAT_ARGS(searchptr)); | ||
| } | ||
|
|
||
| extractPageInfo(xlogreader, true); | ||
| /* We can break if met a safety snapshot */ | ||
| if (base_xid <= min_commited_xid) | ||
| break; | ||
| /* Walk backwards to previous record. */ | ||
| searchptr = record->xl_prev; | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing safety check for infinite backward scan.
If extractPageMapBackward is called when min_commited_xid is InvalidTransactionId (0), the termination condition base_xid <= min_commited_xid at line 177 will never be satisfied by any valid base_xid, causing the function to scan backward until WAL exhaustion or error.
While pg_rewind.c currently guards this with check_rewind_safety(), the function itself should be defensive.
Add an early safety check at the start of extractPageMapBackward:
if (min_commited_xid == InvalidTransactionId)
pg_fatal("extractPageMapBackward called without committed transactions");🛡️ Proposed safety check
extractPageMapBackward(const char *datadir, XLogRecPtr startpoint, int tliIndex,
const char *restoreCommand)
{
+ /* Ensure we have valid XID bounds before scanning backward */
+ if (min_commited_xid == InvalidTransactionId)
+ pg_fatal("extractPageMapBackward called without committed transactions");
+
/* Walk backwards, starting from the given record */
XLogRecord *record;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bin/pg_rewind/parsexlog.c` around lines 159 - 182, The function
extractPageMapBackward lacks a defensive guard against an unset
min_commited_xid; add an early check at the top of extractPageMapBackward that
tests if min_commited_xid == InvalidTransactionId and call pg_fatal with a clear
message (e.g., "extractPageMapBackward called without committed transactions")
to prevent infinite backward WAL scanning; keep the existing termination check
(base_xid <= min_commited_xid) and the rest of the loop unchanged.
| if(!page_consistence && !check_rewind_safety()) | ||
| { | ||
| /* | ||
| * Next time we will search more wal which will spend more time, and | ||
| * if user prefer a deep dig we will do it. Otherwise, we just stop here. | ||
| */ | ||
| if(!deep_dig) | ||
| pg_fatal("Can not safety rewind the target cluster, use --deep-dig to have a try."); | ||
| extractPageMapBackward(datadir_target, chkptrec, lastcommontliIndex, | ||
| restore_command); | ||
| } |
There was a problem hiding this comment.
Fix grammar, style, and error message clarity.
Multiple issues in the conditional backward scan block:
- Grammar error (line 515): "Can not safety rewind" → "Cannot safely rewind"
- Style violation (lines 508, 514): Missing space after
if— should beif (!page_consistenceandif (!deep_dig)per PostgreSQL coding style - Unclear error message (line 515): "have a try" is informal and vague
Consider:
📝 Proposed fix
- if(!page_consistence && !check_rewind_safety())
+ if (!page_consistence && !check_rewind_safety())
{
/*
- * Next time we will search more wal which will spend more time, and
- * if user prefer a deep dig we will do it. Otherwise, we just stop here.
+ * Backward WAL scan is required but will take additional time.
+ * Proceed only if user explicitly requested --deep-dig.
*/
- if(!deep_dig)
- pg_fatal("Can not safety rewind the target cluster, use --deep-dig to have a try.");
+ if (!deep_dig)
+ pg_fatal("cannot safely rewind target cluster without wal_log_hints or checksums; use --deep-dig to enable backward WAL scanning");
extractPageMapBackward(datadir_target, chkptrec, lastcommontliIndex,
restore_command);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bin/pg_rewind/pg_rewind.c` around lines 508 - 518, Fix the conditional
block that handles the backward scan: adjust style to use spaces after if (e.g.,
if (!page_consistence) and if (!deep_dig)), correct grammar and clarity in the
pg_fatal message (replace "Can not safety rewind the target cluster, use
--deep-dig to have a try." with a clearer message such as "Cannot safely rewind
the target cluster; rerun with --deep-dig to attempt a deeper scan."), and leave
the call to extractPageMapBackward(datadir_target, chkptrec, lastcommontliIndex,
restore_command) unchanged when deep_dig is true; retain the
check_rewind_safety() logic and the use of page_consistence and deep_dig flags.
|
Thanks for bringing new feature to IvorySQL! Steven |
|
Thanks for working on this — allowing Correctness Issues (must fix) 1. ABORT records corrupt In 2. Backward scan has no guaranteed termination The loop in 3. XID comparisons are not wraparound-safe
4. The function's comments just say "it's safe" without explaining why. The actual argument depends on PostgreSQL's full-page write (FPW) mechanism: if all post-fork committed transactions started after the last checkpoint, their page modifications all carry FPW records already captured by the forward scan. This reasoning should be documented clearly so reviewers can verify it. Additionally, the function needs a guard for Design Issues 5. Removing the sanity check unconditionally changes default behavior The existing check in 6. XID range filter
7. Global XID trackers are never reset
8. New backend dependencies increase maintenance coupling The patch pulls in Test Coverage The patch adds no regression tests. Given that this bypasses a long-standing safety restriction, tests are essential before merging. At minimum, please add:
|
|
感谢指出代码中的诸多问题，已经根据review意见做出了修改，但是对第6点保持了原来的设计。 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bin/pg_rewind/parsexlog.c`:
- Around line 350-357: The safety check in check_rewind_safety incorrectly uses
the strict wraparound-safe comparison RewindTransactionIdPrecedes(base_xid,
min_commited_xid) which rejects the equal boundary; change this to the inclusive
wraparound-safe helper (e.g. RewindTransactionIdPrecedesOrEquals) or otherwise
perform an inclusive comparison so that base_xid == min_commited_xid is treated
as safe; update the conditional that references base_xid and min_commited_xid
accordingly.
- Around line 569-587: The RM_XLOG_ID checkpoint-handling block (checking
XLOG_CHECKPOINT_SHUTDOWN/ONLINE and updating base_xid) is never reached because
track_rewind_snapshot() returns early for any rmgr other than RM_XACT_ID or
RM_STANDBY_ID; change that early-return logic so RM_XLOG_ID is allowed through.
Update the condition in track_rewind_snapshot() (or the surrounding
early-return) to treat RM_XLOG_ID like RM_XACT_ID/RM_STANDBY_ID (i.e., do not
return for RM_XLOG_ID), so the XLogRecGetInfo/XLogRecGetData checkpoint handling
can run and set base_xid before the downstream safety checks that call
RewindTransactionIdPrecedes. Ensure the change still preserves existing behavior
for other rmgr ids and add a comment noting why RM_XLOG_ID must be processed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ef5af29-0f28-43d3-988e-4586f24453ae
📒 Files selected for processing (2)
src/bin/pg_rewind/parsexlog.csrc/bin/pg_rewind/pg_rewind.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bin/pg_rewind/pg_rewind.c
| bool check_rewind_safety(void) | ||
| { | ||
| /* If no committed transactions, it's safe to rewind */ | ||
| if (!TransactionIdIsValid(min_commited_xid)) | ||
| return true; | ||
| /* If base_xid is less than or equal to min_commited_xid, it's safe */ | ||
| if (TransactionIdIsValid(base_xid) && RewindTransactionIdPrecedes(base_xid, min_commited_xid)) | ||
| return true; |
There was a problem hiding this comment.
Use the inclusive XID comparison here.
Line 356 switched the safety test to the wraparound-safe helper, but it is still strict <. The backward pass stops on base_xid <= min_commited_xid, and pg_rewind.c treats false here as “deep-dig or fail”, so an equal boundary now rejects a safe rewind or forces unnecessary extra WAL scanning.
🩹 Minimal fix
bool check_rewind_safety(void)
{
/* If no committed transactions, it's safe to rewind */
if (!TransactionIdIsValid(min_commited_xid))
return true;
/* If base_xid is less than or equal to min_commited_xid, it's safe */
- if (TransactionIdIsValid(base_xid) && RewindTransactionIdPrecedes(base_xid, min_commited_xid))
+ if (TransactionIdIsValid(base_xid) &&
+ RewindTransactionIdPrecedesOrEquals(base_xid, min_commited_xid))
return true;
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool check_rewind_safety(void) | |
| { | |
| /* If no committed transactions, it's safe to rewind */ | |
| if (!TransactionIdIsValid(min_commited_xid)) | |
| return true; | |
| /* If base_xid is less than or equal to min_commited_xid, it's safe */ | |
| if (TransactionIdIsValid(base_xid) && RewindTransactionIdPrecedes(base_xid, min_commited_xid)) | |
| return true; | |
| bool check_rewind_safety(void) | |
| { | |
| /* If no committed transactions, it's safe to rewind */ | |
| if (!TransactionIdIsValid(min_commited_xid)) | |
| return true; | |
| /* If base_xid is less than or equal to min_commited_xid, it's safe */ | |
| if (TransactionIdIsValid(base_xid) && | |
| RewindTransactionIdPrecedesOrEquals(base_xid, min_commited_xid)) | |
| return true; | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bin/pg_rewind/parsexlog.c` around lines 350 - 357, The safety check in
check_rewind_safety incorrectly uses the strict wraparound-safe comparison
RewindTransactionIdPrecedes(base_xid, min_commited_xid) which rejects the equal
boundary; change this to the inclusive wraparound-safe helper (e.g.
RewindTransactionIdPrecedesOrEquals) or otherwise perform an inclusive
comparison so that base_xid == min_commited_xid is treated as safe; update the
conditional that references base_xid and min_commited_xid accordingly.
| else if (rmid == RM_XLOG_ID) | ||
| { | ||
| info = XLogRecGetInfo(record) & ~XLR_INFO_MASK; | ||
|
|
||
| if (info == XLOG_CHECKPOINT_SHUTDOWN || info == XLOG_CHECKPOINT_ONLINE) | ||
| { | ||
| TransactionId next_xid = InvalidTransactionId; | ||
| char *rec = XLogRecGetData(record); | ||
| CheckPoint *checkpoint = (CheckPoint *) rec; | ||
|
|
||
| next_xid = XidFromFullTransactionId(checkpoint->nextXid); | ||
| if (!TransactionIdIsValid(base_xid) || RewindTransactionIdPrecedes(next_xid, base_xid)) | ||
| { | ||
| base_xid = next_xid; | ||
| if (showprogress) | ||
| pg_log_info("Get next xid: %u at %X/%X", base_xid, LSN_FORMAT_ARGS(record->ReadRecPtr)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new checkpoint fallback is unreachable.
This RM_XLOG branch never runs because track_rewind_snapshot() still returns at Lines 509-510 for any rmgr other than RM_XACT_ID or RM_STANDBY_ID. In histories where a checkpoint is the only available source of nextXid, base_xid stays invalid and both the safety check and backward termination logic misclassify the WAL as needing a deeper scan.
🩹 Minimal fix
- if (rmid != RM_XACT_ID && rmid != RM_STANDBY_ID)
+ if (rmid != RM_XACT_ID &&
+ rmid != RM_STANDBY_ID &&
+ rmid != RM_XLOG_ID)
return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bin/pg_rewind/parsexlog.c` around lines 569 - 587, The RM_XLOG_ID
checkpoint-handling block (checking XLOG_CHECKPOINT_SHUTDOWN/ONLINE and updating
base_xid) is never reached because track_rewind_snapshot() returns early for any
rmgr other than RM_XACT_ID or RM_STANDBY_ID; change that early-return logic so
RM_XLOG_ID is allowed through. Update the condition in track_rewind_snapshot()
(or the surrounding early-return) to treat RM_XLOG_ID like
RM_XACT_ID/RM_STANDBY_ID (i.e., do not return for RM_XLOG_ID), so the
XLogRecGetInfo/XLogRecGetData checkpoint handling can run and set base_xid
before the downstream safety checks that call RewindTransactionIdPrecedes.
Ensure the change still preserves existing behavior for other rmgr ids and add a
comment noting why RM_XLOG_ID must be processed.
|
感谢提交。我看了这个提交，并且把你之前在pg社区提交的讨论也仔细看了一遍，在进入到代码层面之前，有几个问题我想先理解一下：
|
引入问题
PostgreSQL中使用pg_rewind进行脑裂后旧主跟随新主的操作，但是需要开启wal_log_hints或者data_checksums才能使用此功能，这大大降低了pg_rewind的使用范围。
目前依赖wal_log_hints或者data_checksums的原因
在pg_rewind功能引入时，由于需要处理分岔点之前修改数据，分岔点之后修改tuple hint的场景。因此需要开启wal_log_hints或者data_checksums来规避一些场景。如下是如果不开启这两个参数而使用pg_rewind，可能引起的数据不一致问题。
这时如果执行rewind那么, 步骤5的修改不会被rewind察觉，因此不会重新拷贝这些page。这将导致rewind后的A和B的数据不一致。好了，这是社区版本要求wal_log_hints或者data_checksums的理由。现在我们有不开启这两个参数而规避此问题的方法。
新的方案
我们需要记录在分叉点之后提交的所有事务 ID，并在分叉点之前进一步分析 WAL 日志，找到一个起点 LSN（称为 lsn_s），该 LSN 比所有这些事务都更早。然后，我们应该从 lsn_s 读取到分叉点的 LSN，以收集这些事务所影响的页面。这样，我们就可以在 rewind 阶段将这些页面复制过来，以此达到主备一致的目的。
使用介绍
在pg_rewind的使用参数中添加了--deep-dig, 如果不加这个参数将走原生pg_rewind的逻辑，添加这个参数之后，将会走新的rewind逻辑。
Summary by CodeRabbit
New Features
Bug Fixes / Reliability