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
