MIME-Version: 1.0
References: 
 <CAN305gBeJr8m7ZRW9mH0zakEFR4hDUPDo8fJRKJOHWMORG5_Bg@mail.gmail.com>
In-Reply-To: 
 <CAN305gBeJr8m7ZRW9mH0zakEFR4hDUPDo8fJRKJOHWMORG5_Bg@mail.gmail.com>
From: surya poondla <suryapoondla4@gmail.com>
Date: Thu, 21 May 2026 15:09:05 -0700
Message-ID: 
 <CAOVWO5oZZtniLR4Pyd=e_cS-FNxh837Gbz9TDUnSwWqmbap=bw@mail.gmail.com>
Subject: Re: pg_rewind does not rewind diverging timelines
To: Mats Kindahl <mats.kindahl@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: multipart/alternative; boundary="000000000000c6579406525b2942"
Archived-At: 
 <https://www.postgresql.org/message-id/CAOVWO5oZZtniLR4Pyd%3De_cS-FNxh837Gbz9TDUnSwWqmbap%3Dbw%40mail.gmail.com>
Precedence: bulk

--000000000000c6579406525b2942
Content-Type: text/plain; charset="UTF-8"

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.


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.


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?


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.


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
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.
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.


I'm happy to keep reviewing/contributing, thanks again for working on it.

Regards,
Surya Poondla

>

--000000000000c6579406525b2942
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Hi Mats,<br><br>Thanks for picking this u=
p -- the scenario is a real one and I think=C2=A0the UUID-tagging approach =
is a clean way to solve it. v2 applies and=C2=A0builds without trouble, and=
 the core algorithm reads well to me.=C2=A0</div><div dir=3D"ltr">I=C2=A0ha=
ve a handful of observations that I&#39;d love your thoughts.<br><br><br>Re=
garding Correctness I have the below thoughts<br><br>1. UUIDv7 timestamp ep=
och.<br>=C2=A0 =C2=A0 =C2=A0In StartupXLOG():<br>=C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0TimestampTz now =3D GetCurrentTimestamp();<br>=C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0generate_uuidv7_r(&amp;uuid_buf, (uint64)(now / 1000),<br>=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (uint32)(now % =
1000) * 1000);<br><br>I think there might be a small mismatch here: GetCurr=
entTimestamp() returns microseconds since the Postgres epoch (2000-01-01),=
=C2=A0</div><div dir=3D"ltr">whereas generate_uuidv7_r describes its first =
argument as milliseconds since the Unix epoch.=C2=A0</div><div dir=3D"ltr">=
As written that 30-year offset would land in the UUID&#39;s timestamp field=
, so the resulting UUID wouldn&#39;t be a conformant UUIDv7 and wouldn&#39;=
t<br>time-order against UUIDv7s generated through the SQL functions.<br><br=
>Uniqueness is preserved either way, so the rewind logic still works as int=
ended but it seemed worth flagging.<br><br>I see conversion that&#39;s used=
 elsewhere as:<br>us =3D ts + (POSTGRES_EPOCH_JDATE - UNIX_EPOCH_JDATE)<br>=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* SECS=
_PER_DAY * USECS_PER_SEC;<br><br>Or, since promotion isn&#39;t on a hot pat=
h, gettimeofday() / time(NULL) directly would also be fine.<br><br><br>2. E=
OR-record path, the intent is unclear.<br><br>The comment above generate_uu=
idv7_r() at says:<br><br>&quot;The same UUID is written into the history fi=
le and later into the XLOG_END_OF_RECOVERY record so that pg_rewind can dis=
tinguish two servers...&quot;<br><br>But from what I can see only the histo=
ry-file part actually lands.<br>xl_end_of_recovery is unchanged, CreateEndO=
fRecoveryRecord() doesn&#39;t add the UUID, and XLogCtl-&gt;ThisTimeLineUUI=
D is written under info_lck without a</div><div dir=3D"ltr">reader (I could=
n&#39;t grep it). <br><br>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.<br><br>Was the EOR-record piece something you=
 intended to keep for a follow-up, or has it been superseded by the history=
-file approach?<br>=C2=A0 =C2=A0 =C2=A0<br><br>3. Malformed UUID handling i=
n readTimeLineHistory().<br><br>=C2=A0 =C2=A0 =C2=A0The optional field-4 pa=
th is:<br><br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (nfields =3D=3D 4 &amp;&=
amp; strlen(uuid_str) =3D=3D UUID_STR_LEN)<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0{<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Datum datum =3D =
DirectFunctionCall1(uuid_in,<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0CStringGetDatum(uuid_st=
r));<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0...<br>=C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0}<br><br>uuid_in() raises ereport(ERROR) on a malfo=
rmed input, while the surrounding syntax-error paths in readTimeLineHistory=
() use FATAL deliberately. <br>In practice an ERROR during startup ends up =
being fatal too, so this isn&#39;t strictly a bug but it would be nicer to =
stay consistent.=C2=A0<br><br><br>Regarding the Tests I have the following =
thoughts<br><br>The two new cases are nice, a few extensions that I think w=
ould strengthen them:<br>1. A mixed-version case where one side has a zero =
UUID. That&#39;s the path we&#39;re claiming is graceful, but nothing curre=
ntly exercises it<br>2. A deeper-divergence case (e.g. TLI1-&gt;2-&gt;3 vs =
TLI1-&gt;2-&gt;3&#39;) so that findCommonAncestorTimeline&#39;s loop walks =
past matching entries<br>=C2=A0 =C2=A0 =C2=A0before hitting the mismatch. T=
he 0002 test puts the divergence at depth 1.<br>3. A small assertion agains=
t the on-disk 00000002.history contents, to pin down the file format.<br>4.=
 On 0002=C2=A0the dependency on restore_command pointing at node_x&#39;s pg=
_wal is the kind of thing that tends to break under<br>=C2=A0 =C2=A0 =C2=A0=
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.<br><br><br>I&#39;m ha=
ppy to keep reviewing/contributing, thanks again for=C2=A0working on it.<br=
><br>Regards,<br>Surya Poondla</div><div class=3D"gmail_quote gmail_quote_c=
ontainer"><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8=
ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
</blockquote></div></div>

--000000000000c6579406525b2942--