Re: pg_rewind does not rewind diverging timelines

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
>