Re: PITR: enhance getRecordTimestamp()

On Thu, 27 Jan 2022 at 06:58, Michael Paquier <michael@paquier.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/