Re: pg_rewind does not rewind diverging timelines

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.

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.

Regards,
Tatsuya Kawata