Re: PITR: enhance getRecordTimestamp()

On Thu, 27 Jan 2022 at 06:58, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, Nov 03, 2021 at 04:59:04PM +0000, Simon Riggs wrote:
> > Thanks. Fixed and rebased.
>
> + if (xact_info == XLOG_XACT_PREPARE)
> + {
> + if (recoveryTargetUseOriginTime)
> + {
> + xl_xact_prepare *xlrec = (xl_xact_prepare *) XLogRecGetData(record);
> + xl_xact_parsed_prepare parsed;
> +
> + ParsePrepareRecord(XLogRecGetInfo(record),
> + xlrec,
> + &parsed);
> + *recordXtime = parsed.origin_timestamp;
> + }
> + else
> + *recordXtime = ((xl_xact_prepare *) XLogRecGetData(record))->prepared_at;
>
> As I learnt recently with ece8c76, there are cases where an origin
> timestamp may not be set in the WAL record that includes the origin
> timestamp depending on the setup done on the origin cluster. Isn't
> this code going to finish by returning true when enabling
> recovery_target_use_origin_time in some cases, even if recordXtime is
> 0? So it seems to me that this is lacking some sanity checks if
> recordXtime is 0.
>
> Could you add some tests for this proposal? This adds various PITR
> scenarios that would be uncovered, and TAP should be able to cover
> that.

Thanks. Yes, will look at that.

--
Simon Riggs http://www.EnterpriseDB.com/