Hello Zsolt,
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.
Best wishes,
Mats Kindahl
On 6/8/26 21:52, Zsolt Parragi wrote:
> Hello
>
> I know there's still ongoing discussion on the direction itself, but I
> focused on just testing and looking at the latest patch, in case the
> fix remains the same.
>
> + 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.
>
> + memset(&entry->tluuid, 0, sizeof(pg_uuid_t));
> + if (nfields == 4 && strlen(uuid_str) == UUID_STR_LEN)
> + rewind_parse_uuid(uuid_str, &entry->tluuid);
>
> This ignores the return value of rewind_parse_uuid, possibly writing
> partial garbage to tluuid on incorrect input.
>
> Also, it seems like that with this patch, pg rewind requires the
> target's history file to be always there - is this an intended change?
> If yes, then it should be at least mentioned somewhere.
>
> On master:
> exit 0
> "source and target cluster are on the same timeline"
> "no rewind required"
>
> On patched rewind:
> exit 1
> error: could not open file
> ".../tgt/pg_wal/00000002.history" for reading: No such file or directory
That was not the intention, so I added a check for a target history file
to the code and a test that checks the behavior without a timeline
history file and compared that with the behavior before the change.
>
>
> writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
> + const pg_uuid_t *newTLUUID,
> XLogRecPtr switchpoint, char *reason)
>
> In case this is a bug that should be backported, wouldn't this be an ABI break?
>
>
> +#endif /* !FRONTEND */
> +
> +extern pg_uuid_t *generate_uuidv7(uint64 unix_ts_ms, uint32 sub_ms);
> +extern pg_uuid_t *generate_uuidv7_r(pg_uuid_t *uuid, uint64
> unix_ts_ms, uint32 sub_ms);
>
> Shouldn't these go before the endif?
I thought the generate_uuidv7 should be possible to use for a frontend,
but looking at how it is written and linked, that is not possible to I
moved the #endif.
