From: Japin Li <japinli@hotmail.com>
To: Mats Kindahl <mats.kindahl@gmail.com>
Cc: surya poondla <suryapoondla4@gmail.com>,
  pgsql-hackers@lists.postgresql.org
Subject: Re: pg_rewind does not rewind diverging timelines
In-Reply-To: 
 <CAN305gBPFE8KPgT5cdsbK8Xwxxii_+Hp4WVhCWsjFOYJ9j4xaw@mail.gmail.com>
	(Mats Kindahl's message of "Mon, 25 May 2026 20:59:47 +0200")
References: 
 <CAN305gBeJr8m7ZRW9mH0zakEFR4hDUPDo8fJRKJOHWMORG5_Bg@mail.gmail.com>
	<CAOVWO5oZZtniLR4Pyd=e_cS-FNxh837Gbz9TDUnSwWqmbap=bw@mail.gmail.com>
	<CAN305gCcj4Mhr3uBQAnQCYsx6F-syp1rGtazoy=h+_EHO0xOXA@mail.gmail.com>
	<SY7PR01MB1092190B1E748F1438CAB5D7AB60A2@SY7PR01MB10921.ausprd01.prod.outlook.com>
	<CAN305gBPFE8KPgT5cdsbK8Xwxxii_+Hp4WVhCWsjFOYJ9j4xaw@mail.gmail.com>
User-Agent: mu4e 1.14.1; emacs 30.2
Date: Tue, 26 May 2026 14:56:09 +0800
Message-ID: 
 <SY7PR01MB109216DF36CF987CF6EF10B29B60B2@SY7PR01MB10921.ausprd01.prod.outlook.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: SY7PR01MB10921:EE_|SY6PR01MB8458:EE_
X-MS-Office365-Filtering-Correlation-Id: 2364958c-32e5-4b93-8dbf-08debaf3e0e3
X-Microsoft-Antispam: 
	BCL:0;ARA:14566002|41001999006|6090799003|23021999003|8060799015|5072599009|15080799012|19110799012|24021099003|24121999003|22091999003|3412199025|440099028|18061999006|12091999003|26121999007|40105399003|52005399003;
X-Microsoft-Antispam-Message-Info: 
	=?utf-8?B?VG9kS0o1Wmt1WW1xaEZCajUyeVVFVVJWanFMcGlFSG13ZWxEVm1aUUdUNnZ3?=
 =?utf-8?B?c05yYWFGcnJ2MkF5QTdnSXhHT08xRkMyYWhyVFRmVkoraktRSkNwUlNlR0Nz?=
 =?utf-8?B?RDhSaUF1TkhQb0d5bzQ0Y3haZUNjOTlOdUhtdmRuS2tBYUphd05JU3FzY3JW?=
 =?utf-8?B?NzZ1dlY4TkhBMGRjL2tQV05nVnNWR1R3WU1EYTR0SnpRZjBadU1oTTVxQWk0?=
 =?utf-8?B?ajRTTktSRkVkZXY1d2ExSXIvbjF2NmhpY0xyUDI0U3I5Um1LT3FZaWlSdURV?=
 =?utf-8?B?RnR3UWgvSyt3T3BHVE1QdzkwVHdvREw0QXVvRER2Y00xcEt3QkhoMElHWFp5?=
 =?utf-8?B?TkwrUjFHM1hudlE5QURSU3M4NHFCZzJDbExWandiTUdLMWoxSWRKNHJUN3E1?=
 =?utf-8?B?ekJyYWttTDZZa014U1VHTVFJdmg1eG42V1FQbmV5TlBPM1NSdmxnbkV4S0l6?=
 =?utf-8?B?Z2lmNERSSTk2VmpYU05CcWNiellLcjlSY0I1aUdUYVUzVVlhOXcvc0ZiU1Ry?=
 =?utf-8?B?cUlpSjExUUtxdlp4RXp6SjFrTU9nSkxTVnJmamQwVE45NVRSaWF4dGlodmg4?=
 =?utf-8?B?Y2JUNStLMnhCVmRXL25zc0YxTlZTOGowZUd4cmt4MmhlUmFVd081ZGRlOFBI?=
 =?utf-8?B?Z2MwRUU2N1RKZzQrQnoxdU5TQ29pbTREaDhaNVllWGxVWFM2Z1UyektLU21H?=
 =?utf-8?B?cUpkMkRLbDNWWlY2cFZqU1Y4ams1UzJEUUI2WGlvNGZSZW1wbFRUY3VqSmdi?=
 =?utf-8?B?bFlZT213N2tPcGQ1SVdrQkFOT2RTZWc1M1YyYnRrTSs2R3dwTFI0Q0ZVY0tF?=
 =?utf-8?B?Z2tmaDRqblR3b2RTQkM5SEpMZDd6ajdtMVhvaVRGZ3FSTTdOdGdSMHlud0hj?=
 =?utf-8?B?YWtHQXRSZk1YMW4wazA2YlVOQzZnNm42UURpNG1IaWdlOC9EcTlUR05wb3gx?=
 =?utf-8?B?bjdMLzJ4UlNrS1dpV3BPTVh2V3hsZTNPUXRuSEFrdkpFamQybGZaOUhXWURO?=
 =?utf-8?B?RzJobnMzdTA2OUpaNHNLeGEzUHh5RzNRVkViVG5vRzN3cHpPWmhLV0FPQjcz?=
 =?utf-8?B?QmlYa1crZ1dSZDdmaE5VZENmRnRqSDBQOVNrY3FKVm9BcGdPU24wNGFCU3Vr?=
 =?utf-8?B?S1RQT2lwWEZDcWdsZWFnbXFSRmk2OElrSWRzY0NaWUpTUTJhdERYa1FjV1VF?=
 =?utf-8?B?NUVUeUYwdXA2dmJqL2lwbmhiWUpiakYyUkNqWWpMa0VSUEpCYUlLYStGR3NQ?=
 =?utf-8?B?aTVOQ3QxQ1RoaXUrK1VkQ3VJeWE3cVZNUTNSWGVUVUtVcTEwMlk2TzFQcXov?=
 =?utf-8?B?QmI4YitON0padG14ZEVVN3l3TU1Dd3M5cG5tK3ByWWtxdEJ0TjF5VC9UbldP?=
 =?utf-8?B?TTl0MWRLRXJIVk56V2R2M3ErSm15dHBqSGh6WWkrYnQ2dTV2bTQ0WkluRmxN?=
 =?utf-8?B?Wkg5VSs1Rk10Q2NBV2pPdTIyNkEwNGx3RW8xbVBpTTM1MDNESm5KdGZrU3k4?=
 =?utf-8?B?dEFLZjhRcklVQ3puUi9SaUdxWDlKa3NFZzdpWEVJSWtzcVEzL2xTSUlEK0Zu?=
 =?utf-8?Q?5bSBxDnwowvKuQ3DutbJ1zKLU=3D?=
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?RXpGU1pPVklYZUZEYStGcEd0ZXVMQUhCL00rNWo5YVZ4b2JKZDBwa0JJdHMr?=
 =?utf-8?B?Q2ROdmJZUFNPRDlMa1VBNGpLbjZRUEx3eis5aW9nRnFNa2tWRjJ4SXBneDM4?=
 =?utf-8?B?eml4TGNHYS9OYmkzandoL25meGdUOWF5SWNib1krcnZYSG1ES1ZXZlZWQzZs?=
 =?utf-8?B?Tk1iU3E5a1FhcG9XbkdOcUQ3Y1oreXlzWDVDRm42RWEvVnltWjdFcGM4RS8v?=
 =?utf-8?B?bWFWcnJZYXdneTlyOWhkTENiSTF5MFFJUkthbGs5ek9MbWVYNWdEcGZ2eUZK?=
 =?utf-8?B?aDhkTjRmdDR2ekNmY2w2Y1A4K2o0NzJVRzBpMytkY2FFcGpGWTJrM093MjA2?=
 =?utf-8?B?bVpsRndROWtSTUtRTVJ3Y1BEWWJaSFJhZFBQNDU1eDFad0JRSTVmZTVScmhT?=
 =?utf-8?B?Rk5Qc3JzZ25URDliblRvWW9CL2YraW5DMWZqd3pRQ1Y1SnJhR0ZkMjcyNWZH?=
 =?utf-8?B?UzNsYjlGREtTUUxPa0Nwdi9jQ1lzZkZKTVFqcUpTZVR4U2tjamNaNE5QZm1n?=
 =?utf-8?B?a2tNTjhzR2F4MkRrYTkrcUp5V3MwNy96cHM0Zkc4S1VhZUt2RVJlazJSOXdR?=
 =?utf-8?B?NGxhZTJ2VGxYaUliaDRXdzFqZmMrZkFqMUdOaG5hd0xwbFpPMHEzcVNTRFJm?=
 =?utf-8?B?czh3Z01EWlBYV1ZrUUZ6cnFQclBySnl3MFIxNzJaSDVUcTBaTlZvWElrV0ZQ?=
 =?utf-8?B?L0NYRFNwVE5ra2Nic1N0cEh1d1h4Nk1PcjE4YnBCUk1KdjRWdTlSMkM4YmNR?=
 =?utf-8?B?WFZFM3pVTnRwQ2RvelFjdUYzb29qbnlhQzE1SjJTUXRHZXpWR1ZocW00dE1S?=
 =?utf-8?B?aFZXdFBncFVwbVRJd2NIK3M4VG1DbDlTRTZrb0RCZ2gwZm5zeFZLdkdoV3dZ?=
 =?utf-8?B?aGk0MzR0MDFQY1hIRVM2NnFiSDYyYWxNenNGYnE0WUN6K0VDdFppdWFZYU9w?=
 =?utf-8?B?YUdqd3lsWnV0MEI2dHhBMmkrWEN0TGgrUVlnaGNMVEd0VGFlN3J6MVE4dWd5?=
 =?utf-8?B?TThnLzlsRDc4Y3hXdkpuWXdQTkxEOW5Md3ZramJZMTc1WnZ5bFk5bFhyTjdu?=
 =?utf-8?B?U1ZJVWdHOGVVT1VKa2xnRW1NMHZnV0JZMC9FT1hHUitMM3FrdThVMjBOKy9X?=
 =?utf-8?B?OUxtbmJmYU8rNy96c2l5MnlKb3dOUHIvYkk0MUtmWU00NjBvRTlJQzdFZGp3?=
 =?utf-8?B?M1MzQmpqcXNyMjNCVG5zQm1VbGZ1V2llY0wxQ294MkVOeWxxT0p5alh3bU9o?=
 =?utf-8?B?cVNDcWZNYUtGd3drS2czMSt4eDM2aHBkL1lFOFVnVjA0blZvVnhCUEJWUkFD?=
 =?utf-8?B?bTZrbC90NXRpSU5iMDJXODVQODhUWXAxOHJWNkljcXp0UTYwKyt4SzlyY3Rp?=
 =?utf-8?B?cU44R0U1SGdjYnZReVgxbnp3M2pqMVdaV0pJbWU2LzJCa2psRkVCMzI0NHc5?=
 =?utf-8?B?M2dEZUNKT3VsRDk5V3hheWhabUlnSjUzVTNlVW01cXhXTFZQaUJ2TWFBY1ZM?=
 =?utf-8?B?ZkJLSFZoMmh3SkFGd3dBcE9jRVlsQ2Znd0tUVC9UMmR5aURKR05ibVQ3V0J3?=
 =?utf-8?B?TExaa3Vzd3h3a0JxazdsM1dxUFc0cGFMbzdSakxxZ29xSGVMcFdxL2k3YVBL?=
 =?utf-8?B?QXo4emtIa2JDZ2FaRllYcUxMdjNzZmZ3R3MvRDM2UzRvVkZ5SmwybUd5dFFB?=
 =?utf-8?B?U25rMnpKaEJvK2tVZm5vZHNHcDQ0eElPQURmd3JOR3hoUWpaOUtWZEh6SmZ0?=
 =?utf-8?B?cWd6NDNvTHdYWkxHdzFxdG5SRituZG51RG5hazdSZEs0U3MwTjlhVkxkNUV2?=
 =?utf-8?B?WGxxTXBhUTYwVUJ0WVJwUDQwRExteUw0Sml6TnpoTmFwRVVJejUrNHN4dEln?=
 =?utf-8?Q?+sju1EqgJEkep?=
X-OriginatorOrg: sct-15-20-9412-4-msonline-outlook-feddd.templateTenant
X-MS-Exchange-CrossTenant-Network-Message-Id: 
 2364958c-32e5-4b93-8dbf-08debaf3e0e3
X-MS-Exchange-CrossTenant-AuthSource: SY7PR01MB10921.ausprd01.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 May 2026 06:56:14.8677
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 
	00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SY6PR01MB8458
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: 
 <https://www.postgresql.org/message-id/SY7PR01MB109216DF36CF987CF6EF10B29B60B2%40SY7PR01MB10921.ausprd01.prod.outlook.com>
Precedence: bulk


Hi, Mats

Thanks for updating the patch.

On Mon, 25 May 2026 at 20:59, Mats Kindahl <mats.kindahl@gmail.com> wrote:
> Hi Japin,
>
> On Mon, May 25, 2026 at 7:21=E2=80=AFAM Japin Li <japinli@hotmail.com> wr=
ote:
>
>  Hi, Mats
>
>  On Sun, 24 May 2026 at 20:30, Mats Kindahl <mats.kindahl@gmail.com> wrot=
e:
>  > On Fri, May 22, 2026 at 12:09=E2=80=AFAM 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 algorit=
hm reads well to me.=20
>  >  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.
>  > =20
>  >  Regarding Correctness I have the below thoughts
>  >
>  >  1. UUIDv7 timestamp epoch.
>  >       In StartupXLOG():
>  >           TimestampTz now =3D GetCurrentTimestamp();
>  >           generate_uuidv7_r(&uuid_buf, (uint64)(now / 1000),
>  >                                        (uint32)(now % 1000) * 1000);
>  >
>  >  I think there might be a small mismatch here: GetCurrentTimestamp() r=
eturns microseconds since the Postgres epoch
>  >  (2000-01-01),=20
>  >  whereas generate_uuidv7_r describes its first argument as millisecond=
s since the Unix epoch.=20
>  >  As written that 30-year offset would land in the UUID's timestamp fie=
ld, so the resulting UUID wouldn't be a
>  >  conformant UUIDv7 and wouldn't
>  >  time-order against UUIDv7s generated through the SQL functions.
>  >
>  > =20
>  > =20
>  >  Uniqueness is preserved either way, so the rewind logic still works a=
s intended but it seemed worth flagging.
>  >
>  >  I see conversion that's used elsewhere as:
>  >  us =3D 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 se=
rvers if necessary. Switched to gettimeofday
>  () and
>  > used 0 for sub-ms since this is not going to be critical. (We could us=
e 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 un=
likely, and there is a random number added
>  for that
>  > situation.)
>  > =20
>  >  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 XL=
OG_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).=20
>  >
>  >  The xlog_redo() memset() + Min(rec_len, sizeof(...)) change reads lik=
e 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).
>  > =20
>  >      =20
>  >
>  >  3. Malformed UUID handling in readTimeLineHistory().
>  >
>  >       The optional field-4 path is:
>  >
>  >           if (nfields =3D=3D 4 && strlen(uuid_str) =3D=3D UUID_STR_LEN=
)
>  >           {
>  >               Datum datum =3D DirectFunctionCall1(uuid_in,
>  >                                                 CStringGetDatum(uuid_s=
tr));
>  >               ...
>  >           }
>  >
>  >  uuid_in() raises ereport(ERROR) on a malformed input, while the surro=
unding syntax-error paths in
>  readTimeLineHistory
>  >  () use FATAL deliberately.=20
>  >  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 (wi=
th the error message from the uuid_in, in
>  case it
>  > is modified it makes sense to show this).
>  > =20
>  >  Regarding the Tests I have the following thoughts
>  >
>  >  The two new cases are nice, a few extensions that I think would stren=
gthen them:
>  >  1. A mixed-version case where one side has a zero UUID. That's the pa=
th we're claiming is graceful, but nothing
>  >  currently exercises it
>  >
>  > Yes, that should work regardless of whether the source or the target h=
as 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 neces=
sary changes in the attached patches for
>  testing.
>  > Please have a look.
>  > =20
>  >  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 a=
t depth 1.
>  >
>  > I was unsure if this test was necessary or interesting, hence a separa=
te 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 b=
elow.
>  > =20
>  >  3. A small assertion against the on-disk 00000002.history contents, t=
o 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, o=
r 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 d=
o the same job as the primitives I wrote, but
>  have
>  > hard-coded variable names. I could rewrite them to take parameters, bu=
t 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=E2=9A=A0=EF=B8=8F=E2=9A=A0=EF=B8=8F.
>  > =20
>  > 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 =3D &tlh->source[tlh->sourceNentries - 2].tluuid;
>  +       b =3D &tlh->target[tlh->targetNentries - 2].tluuid;
>  +
>  +       if (memcmp(a, &zero, UUID_LEN) =3D=3D 0 && memcmp(b, &zero, UUID=
_LEN) =3D=3D 0)
>  +               return true;
>  +
>  +       return memcmp(a, b, UUID_LEN) =3D=3D 0;
>
>  Since we already have matchingTimelineUUID(), the above code can be simp=
lified
>  using it.
>
> Thank you for the review. I switched to using the matchingTimelineUUID() =
for this part of the code and made some other
> minor improvements as well.

Here are some comments on v4.

1.
+/*
+ * Timeline histories for both clusters, populated by timelines_match().
+ */

I don't see a timelines_match() function.  Does this refer to
matchAndFetchTimelines()?

2.
+typedef struct TimelineHistoriesData
+{
+       TimeLineHistoryEntry *source,
+                          *target;
+       int                     sourceNentries,
+                               targetNentries;
+}                      TimelineHistoriesData;

I'd prefer to use TimeLineHistoriesData to stay consistent with
TimeLineHistoryEntry.  Anyway I'm not instant on it.

3.
+typedef TimelineHistoriesData * TimelineHistories;

The space between * and TimelineHistories is unnecessary =E2=80=94 see
StringInfoData and other typedefs.

4.
+# node_x and node_b both start from the same TLI 1 baseline.
+my ($node_x, $node_b2) =3D
+  setup_standbys_from_origin($node_origin2, 'node_x', 'node_b2');

There appears to be a typo in the comment.  The node_b should be node_b2.


Everything else looks good.  Thank you again for updating the patch!

> --
> Best wishes,
> Mats Kindahl, Multigres Developer, Supabase

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