Обсуждение: PITR: enhance getRecordTimestamp()

For PITR, getRecordTimestamp() did not include all record types that
contain times.
Add handling for checkpoints, end of recovery and prepared xact record types.
Based on earlier discussions with community members.

Also, allow the option of recovery_target_use_origin_time = off (default) | on.
This allows PITR to consider whether it should use the local server
time of changes, or whether it should use the origin time on each
node. This is useful in multi-node data recovery.

This is part of a series of enhancements to PITR, in no specific order.

Passes make check and recovery testing; includes docs.

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

> On 30 Jun 2021, at 11:59, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

> For PITR, getRecordTimestamp() did not include all record types that
> contain times.
> Add handling for checkpoints, end of recovery and prepared xact record types.

+     <variablelist>
This breaks doc compilation, and looks like a stray tag as you want this entry
in the currently open variablelist?

--
Daniel Gustafsson        https://vmware.com/

On Wed, 3 Nov 2021 at 13:28, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 30 Jun 2021, at 11:59, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
>
> > For PITR, getRecordTimestamp() did not include all record types that
> > contain times.
> > Add handling for checkpoints, end of recovery and prepared xact record types.
>
> +     <variablelist>
> This breaks doc compilation, and looks like a stray tag as you want this entry
> in the currently open variablelist?

Thanks. Fixed and rebased.

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

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.
--
Michael

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/