Message-ID: <0ed702a2-5cb2-4d5b-a9e2-c9ae66fe7c83@gmail.com>
Date: Sun, 16 Aug 2026 13:37:32 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: pg_rewind does not rewind diverging timelines
To: Tatsuya Kawata <kawatatatsuya0913@gmail.com>
Cc: Zsolt Parragi <zsolt.parragi@percona.com>,
 pgsql-hackers@lists.postgresql.org
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>
 <SY7PR01MB109216DF36CF987CF6EF10B29B60B2@SY7PR01MB10921.ausprd01.prod.outlook.com>
 <CAN305gCaErXmG3fg48n50dWUC7=ETBBopuFL_cgyzutXUdp-5g@mail.gmail.com>
 <SY7PR01MB10921E0F8383139EB27B33C11B6162@SY7PR01MB10921.ausprd01.prod.outlook.com>
 <9ce0d2b9-7a41-4a8a-b299-da295bb4514f@gmail.com>
 <CAN4CZFO+HVWRWELsu4CuQ+Fr=J4e+jdHM5siRQ3G3vyJ5-Y6Sg@mail.gmail.com>
 <8683af69-28af-4a2d-a1db-aa1447b02446@gmail.com>
 <CAHza6qczQXOwEyqNX2rq5UNo2XQb_bWY33F=9sNXzVPr6DztMA@mail.gmail.com>
Content-Language: en-US
From: Mats Kindahl <mats.kindahl@gmail.com>
In-Reply-To: 
 <CAHza6qczQXOwEyqNX2rq5UNo2XQb_bWY33F=9sNXzVPr6DztMA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/0ed702a2-5cb2-4d5b-a9e2-c9ae66fe7c83%40gmail.com>
Precedence: bulk

On 7/26/26 11:57, Tatsuya Kawata wrote:
> Hi Mats-san, Zsolt-san,
>
> Thanks -- I went through both v7 and the new version.
>
> >   +                       PG_CATCH();
> >   +                       {
> >   +                               ErrorData  *edata = CopyErrorData();
> >   +
> >   +                               FlushErrorState();
> >   +                               ereport(FATAL,
> >   +  errmsg("invalid UUID in history file \"%s\"", path),
> >   +  errdetail("%s", edata->message));
> >   +                       }
> >
> >   This is missing a MemoryContextSwitchTo before CopyErrorData, and
> >   results in an assertion with debug builds.
>
> > Thank you for reviewing this and sorry for the delay. I have attached a
> > new version with the issues you pointed to handled. See comments inline
> > below.
>
> The context-switch
> fix in readTimeLineHistory() (restoring the caller's context before
> CopyErrorData()) looks correct to me.
>
> One note: the original problem was not only a debug-build assertion. On
> non-assert builds CopyErrorData() allocates the ErrorData in ErrorContext,
> FlushErrorState() then frees it, and the following
> errdetail("%s", edata->message) reads freed memory -- a use-after-free 
> that
> can crash a production server, not just trip an Assert(). Your fix already
> covers this; I'm just sharing it since it bears on the severity.

Got that. Assertions are just a way to trigger a potential problem 
early. I did not assume this change was needed just to avoid the assertion.

> One minor point: on an invalid UUID the backend FATALs while the frontend
> (pg_rewind) silently treats it as "unknown" (all-zero) -- probably
> intentional, just flagging it.And should you ever want to drop the
> PG_TRY/PG_CATCH here, uuid_in supports soft errors, so a
> DirectInputFunctionCallSafe() call with an ErrorSaveContext would avoid
> CopyErrorData()/FlushErrorState() and the context switch entirely -- i.e.
> it removes the very handling that had to be fixed here, so this class of
> mistake can't recur. The current fix is correct and minimal, so this is
> purely optional.

Yes, I wanted to keep the UUID just as a final discriminator, after the 
TLI, and keep the changes minimal.

Best wishes,
Mats Kindahl

>
> Regards,
> Tatsuya Kawata
>