Thread

Re: pg_rewind does not rewind diverging timelines

Japin Li <japinli@hotmail.com> — 2026-05-25T05:20:57Z

Hi, Mats

On Sun, 24 May 2026 at 20:30, Mats Kindahl <mats.kindahl@gmail.com> wrote:
> On Fri, May 22, 2026 at 12:09 AM surya poondla <suryapoondla4@gmail.com> wrote:
>
>  Hi Mats,
>
>  Thanks for picking this up -- the scenario is a real one and I think the UUID-tagging approach is a clean way to
>  solve it. v2 applies and builds without trouble, and the core algorithm reads well to me. 
>  I have a handful of observations that I'd love your thoughts.
>
> Hi Surya,
>
> Thank you for the review. It is a quite rare scenario, but it is real and the fix is simple.
>  
>  Regarding Correctness I have the below thoughts
>
>  1. UUIDv7 timestamp epoch.
>       In StartupXLOG():
>           TimestampTz now = GetCurrentTimestamp();
>           generate_uuidv7_r(&uuid_buf, (uint64)(now / 1000),
>                                        (uint32)(now % 1000) * 1000);
>
>  I think there might be a small mismatch here: GetCurrentTimestamp() returns microseconds since the Postgres epoch
>  (2000-01-01), 
>  whereas generate_uuidv7_r describes its first argument as milliseconds since the Unix epoch. 
>  As written that 30-year offset would land in the UUID's timestamp field, so the resulting UUID wouldn't be a
>  conformant UUIDv7 and wouldn't
>  time-order against UUIDv7s generated through the SQL functions.
>
>  
>  
>  Uniqueness is preserved either way, so the rewind logic still works as intended but it seemed worth flagging.
>
>  I see conversion that's used elsewhere as:
>  us = ts + (POSTGRES_EPOCH_JDATE - UNIX_EPOCH_JDATE)
>                     * SECS_PER_DAY * USECS_PER_SEC;
>
>  Or, since promotion isn't on a hot path, gettimeofday() / time(NULL) directly would also be fine.
>
> Yes, the intention was to use a proper timestamp to allow debugging servers if necessary. Switched to gettimeofday() and
> used 0 for sub-ms since this is not going to be critical. (We could use ns here as well, but that would only solve a race
> if you have two servers being promoted in the same ms, which I find unlikely, and there is a random number added for that
> situation.)
>  
>  2. EOR-record path, the intent is unclear.
>
>  The comment above generate_uuidv7_r() at says:
>
>  "The same UUID is written into the history file and later into the XLOG_END_OF_RECOVERY record so that pg_rewind can
>  distinguish two servers..."
>
>  But from what I can see only the history-file part actually lands.
>  xl_end_of_recovery is unchanged, CreateEndOfRecoveryRecord() doesn't add the UUID, and XLogCtl->ThisTimeLineUUID is
>  written under info_lck without a
>  reader (I couldn't grep it). 
>
>  The xlog_redo() memset() + Min(rec_len, sizeof(...)) change reads like preparation for an EOR-struct extension that
>  ended up not being part of the patch.
>
>  Was the EOR-record piece something you intended to keep for a follow-up, or has it been superseded by the
>  history-file approach?
>
> No, the EOR changes are not needed for the promotion, contrary to what I originally thought. Cleaned up the comment and
> the code and removed all traces of changes to the EOR (I hope).
>  
>       
>
>  3. Malformed UUID handling in readTimeLineHistory().
>
>       The optional field-4 path is:
>
>           if (nfields == 4 && strlen(uuid_str) == UUID_STR_LEN)
>           {
>               Datum datum = DirectFunctionCall1(uuid_in,
>                                                 CStringGetDatum(uuid_str));
>               ...
>           }
>
>  uuid_in() raises ereport(ERROR) on a malformed input, while the surrounding syntax-error paths in readTimeLineHistory
>  () use FATAL deliberately. 
>  In practice an ERROR during startup ends up being fatal too, so this isn't strictly a bug but it would be nicer to
>  stay consistent.
>
> Agree. I added code to capture the error and raise a FATAL instead (with the error message from the uuid_in, in case it
> is modified it makes sense to show this).
>  
>  Regarding the Tests I have the following thoughts
>
>  The two new cases are nice, a few extensions that I think would strengthen them:
>  1. A mixed-version case where one side has a zero UUID. That's the path we're claiming is graceful, but nothing
>  currently exercises it
>
> Yes, that should work regardless of whether the source or the target has the zero UUID.
>
> I realized one thing: if two timelines have identical TLI but one has zero UUID and one has not, it seems they could not
> come from the same promotion (one promotion happened on an old server and the other one on a new server), that is, they
> should be treated as different. Does that make sense? I made the necessary changes in the attached patches for testing.
> Please have a look.
>  
>  2. A deeper-divergence case (e.g. TLI1->2->3 vs TLI1->2->3') so that findCommonAncestorTimeline's loop walks past
>  matching entries
>       before hitting the mismatch. The 0002 test puts the divergence at depth 1.
>
> I was unsure if this test was necessary or interesting, hence a separate commit. Since you thought it was useful, it's
> now rolled into the patch and I extended the tests with the scenarios you suggested.
>
> I also did some refactorings of the tests to avoid duplication. More below.
>  
>  3. A small assertion against the on-disk 00000002.history contents, to pin down the file format.
>  4. On 0002 the dependency on restore_command pointing at node_x's pg_wal is the kind of thing that tends to break
>  under
>       environment changes. A CHECKPOINT on node_x before the backup, or wal_keep_size as in 0001, would let the test
>  stand on its own.
>
> Good point.
>
> I refactored the code to avoid some duplication and make the test flow self-explanatory and as part of that I set the
> wal_keep_size for all nodes.
>
> In the process I noticed that many of the functions in RewindTest.pm do the same job as the primitives I wrote, but have
> hard-coded variable names. I could rewrite them to take parameters, but that would be quite a big patch to add additional
> changes to each call site, so I did not do that and rather added small wrappers specific for the tests in
> 005_same_timeline.pl⚠️.
>  
> Attached a new version of the now single patch.
>
>  I'm happy to keep reviewing/contributing, thanks again for working on it.
>
> Thank you for reviewing it.

Thank you for your work.  I have one comment.

+	a = &tlh->source[tlh->sourceNentries - 2].tluuid;
+	b = &tlh->target[tlh->targetNentries - 2].tluuid;
+
+	if (memcmp(a, &zero, UUID_LEN) == 0 && memcmp(b, &zero, UUID_LEN) == 0)
+		return true;
+
+	return memcmp(a, b, UUID_LEN) == 0;

Since we already have matchingTimelineUUID(), the above code can be simplified
using it.

-- 
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.