Horton

Thread

Commits

  1. Fix backup manifests to generate correct WAL-Ranges across timelines

  1. pg_veryfybackup can fail with a valid backup for TLI > 1

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-08-18T05:30:31Z

    Hello.

pg_veryfybackup can fail with a valid backup when the backup was taken
from TLI > 1.

=====
# initdb
$ pg_ctl start (just to make sure)
$ pg_ctl stop
$ touch $PGDATA/standby.signal
$ pg_ctl start
$ pg_ctl promote
$ psql
postgres=# select pg_switch_wal();
 pg_switch_wal 
---------------
 0/14FE340
(1 row)
postgres=# checkpoint;
postgres=# ^D
$ pg_basebackup -Fp -h /tmp -D tmpbk
$ pg_veryfybackup tmpbk
(thinking.. thiking.. zzz.. for several seconds)
pg_waldump: fatal: could not find file "000000020000000000000001": No such file or directory
pg_verifybackup: error: WAL parsing failed for timeline 2
=====

This is bacause the backup_manifest has the wrong values as below.

> "WAL-Ranges": [
> { "Timeline": 2, "Start-LSN": "0/14FE248", "End-LSN": "0/3000100" }
> ],

The "Start-LSN" above is the beginning of timeline 2, not the
backup-start LSN. The segment had been removed by the checkpoint.


The comment for AddWALInfoToBackupManifest() says:
> * Add information about the WAL that will need to be replayed when restoring
> * this backup to the manifest.

So I concluded that it's a thinko.

Please see the attached.  It needs to be back-patched to 13 but 13
doesn't accept the patch as is due to wording chages in TAP tests.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  2. Re: pg_veryfybackup can fail with a valid backup for TLI > 1

    Michael Paquier <michael@paquier.xyz> — 2021-08-19T06:51:38Z

    On Wed, Aug 18, 2021 at 02:30:31PM +0900, Kyotaro Horiguchi wrote:
> The "Start-LSN" above is the beginning of timeline 2, not the
> backup-start LSN. The segment had been removed by the checkpoint.

Good catch.  That's broken, and I find cleaner the logic to compare
the TLI numbers rather than the start or end position of the TLI to
check if a TLI should have a parent one.

> The comment for AddWALInfoToBackupManifest() says:
> > * Add information about the WAL that will need to be replayed when restoring
> > * this backup to the manifest.

Yes, backup manifests include one array with all the timelines that
require coverage.  This case would only impact backups taken from
standbys, which have timeline jumps while the backup is taken with
parent nodes promoted.  That's easy enough to test with a chain of
standbys, some pgbench and pg_basebackup --max-rate to slow down the
whole. 

> So I concluded that it's a thinko.
> 
> Please see the attached.  It needs to be back-patched to 13 but 13
> doesn't accept the patch as is due to wording chages in TAP tests.

+       if (starttli == entry->tli)
            tl_beginptr = startptr;
+       else
+       {
+           tl_beginptr = entry->begin;

I would add a comment here.  This does not need to be complicated,
say:
"If this timeline entry matches with the timeline on which the backup
started, WAL needs to be checked from the start LSN of the backup.  If
this entry refers to a newer timeline, WAL needs to be checked since
the beginning of this timeline, so use the LSN where the timeline
began."

The TAP test is fancy, I like it.  So let's keep it.

+# base-backup runs a checkpoint but just to make sure.
+$primary->safe_psql('postgres', 'SELECT pg_switch_wal();
CHECKPOINT;');
I don't think you need this part to get a failure as the checkpoint
from the base backup will be sufficient.
--
Michael

  3. Re: pg_veryfybackup can fail with a valid backup for TLI > 1

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-08-20T06:33:37Z

    Thank you for the comment.

At Thu, 19 Aug 2021 15:51:38 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Wed, Aug 18, 2021 at 02:30:31PM +0900, Kyotaro Horiguchi wrote:
> > The "Start-LSN" above is the beginning of timeline 2, not the
> > backup-start LSN. The segment had been removed by the checkpoint.
> 
> Good catch.  That's broken, and I find cleaner the logic to compare
> the TLI numbers rather than the start or end position of the TLI to
> check if a TLI should have a parent one.
> 
> > The comment for AddWALInfoToBackupManifest() says:
> > > * Add information about the WAL that will need to be replayed when restoring
> > > * this backup to the manifest.
> 
> Yes, backup manifests include one array with all the timelines that
> require coverage.  This case would only impact backups taken from
> standbys, which have timeline jumps while the backup is taken with
> parent nodes promoted.  That's easy enough to test with a chain of
> standbys, some pgbench and pg_basebackup --max-rate to slow down the
> whole. 

(I was very annoyed that pg_basbackup responds to --max-rate=1 only by
"out of range" without informing of the valid range..)

That's looks like a domino falling. I had the following result, which
looks fine.

"WAL-Ranges": [
{ "Timeline": 6, "Start-LSN": "0/630C7E8", "End-LSN": "0/630C850" },
{ "Timeline": 5, "Start-LSN": "0/630C780", "End-LSN": "0/630C7E8" },
{ "Timeline": 4, "Start-LSN": "0/630C718", "End-LSN": "0/630C780" },
{ "Timeline": 3, "Start-LSN": "0/630C6B0", "End-LSN": "0/630C718" },
{ "Timeline": 2, "Start-LSN": "0/5000028", "End-LSN": "0/630C6B0" }
],

00000006.history:
1	0/173F268	no recovery target specified
2	0/630C6B0	no recovery target specified
3	0/630C718	no recovery target specified
4	0/630C780	no recovery target specified
5	0/630C7E8	no recovery target specified

> > So I concluded that it's a thinko.
> > 
> > Please see the attached.  It needs to be back-patched to 13 but 13
> > doesn't accept the patch as is due to wording chages in TAP tests.
> 
> +       if (starttli == entry->tli)
>             tl_beginptr = startptr;
> +       else
> +       {
> +           tl_beginptr = entry->begin;
> 
> I would add a comment here.  This does not need to be complicated,
> say:
> "If this timeline entry matches with the timeline on which the backup
> started, WAL needs to be checked from the start LSN of the backup.  If
> this entry refers to a newer timeline, WAL needs to be checked since
> the beginning of this timeline, so use the LSN where the timeline
> began."

Thanks for your help! I wanted to write something like that
there. Added it as-is.

> The TAP test is fancy, I like it.  So let's keep it.
> 
> +# base-backup runs a checkpoint but just to make sure.
> +$primary->safe_psql('postgres', 'SELECT pg_switch_wal();
> CHECKPOINT;');
> I don't think you need this part to get a failure as the checkpoint
> from the base backup will be sufficient.

Yes, it is just redandunt as (I tried to) said in the comment. (I
meant by the "just to make sure" "just to let readers of the test code
notice that a checkpoint is crucial there", but it doesn't seem
working:() Instead, I added a comment just before pg_basebackup.

# base-backup below runs a checkpoint, which removes the first segment
# of the current timeline

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  4. Re: pg_veryfybackup can fail with a valid backup for TLI > 1

    Michael Paquier <michael@paquier.xyz> — 2021-08-20T07:23:56Z

    On Fri, Aug 20, 2021 at 03:33:37PM +0900, Kyotaro Horiguchi wrote:
> That's looks like a domino falling. I had the following result, which
> looks fine.
> 
> "WAL-Ranges": [
> { "Timeline": 6, "Start-LSN": "0/630C7E8", "End-LSN": "0/630C850" },
> { "Timeline": 5, "Start-LSN": "0/630C780", "End-LSN": "0/630C7E8" },
> { "Timeline": 4, "Start-LSN": "0/630C718", "End-LSN": "0/630C780" },
> { "Timeline": 3, "Start-LSN": "0/630C6B0", "End-LSN": "0/630C718" },
> { "Timeline": 2, "Start-LSN": "0/5000028", "End-LSN": "0/630C6B0" }
> ],
> 
> 00000006.history:
> 1	0/173F268	no recovery target specified
> 2	0/630C6B0	no recovery target specified
> 3	0/630C718	no recovery target specified
> 4	0/630C780	no recovery target specified
> 5	0/630C7E8	no recovery target specified

And your backup_label shows 0/5000028 as start LSN for the backup,
right?  I see the same result.
--
Michael

  5. Re: pg_veryfybackup can fail with a valid backup for TLI > 1

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-08-20T07:47:15Z

    At Fri, 20 Aug 2021 16:23:56 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Fri, Aug 20, 2021 at 03:33:37PM +0900, Kyotaro Horiguchi wrote:
> > That's looks like a domino falling. I had the following result, which
> > looks fine.
> > 
> > "WAL-Ranges": [
> > { "Timeline": 6, "Start-LSN": "0/630C7E8", "End-LSN": "0/630C850" },
> > { "Timeline": 5, "Start-LSN": "0/630C780", "End-LSN": "0/630C7E8" },
> > { "Timeline": 4, "Start-LSN": "0/630C718", "End-LSN": "0/630C780" },
> > { "Timeline": 3, "Start-LSN": "0/630C6B0", "End-LSN": "0/630C718" },
> > { "Timeline": 2, "Start-LSN": "0/5000028", "End-LSN": "0/630C6B0" }
> > ],
> > 
> > 00000006.history:
> > 1	0/173F268	no recovery target specified
> > 2	0/630C6B0	no recovery target specified
> > 3	0/630C718	no recovery target specified
> > 4	0/630C780	no recovery target specified
> > 5	0/630C7E8	no recovery target specified
> 
> And your backup_label shows 0/5000028 as start LSN for the backup,
> right?  I see the same result.

Yes, backup_label looks correct.

backup_label (extract):
START WAL LOCATION: 0/5000028 (file 000000020000000000000005)
CHECKPOINT LOCATION: 0/5000060
START TIMELINE: 2

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  6. Re: pg_veryfybackup can fail with a valid backup for TLI > 1

    Michael Paquier <michael@paquier.xyz> — 2021-08-23T06:46:37Z

    On Fri, Aug 20, 2021 at 04:47:15PM +0900, Kyotaro Horiguchi wrote:
> Yes, backup_label looks correct.
> 
> backup_label (extract):
> START WAL LOCATION: 0/5000028 (file 000000020000000000000005)
> CHECKPOINT LOCATION: 0/5000060
> START TIMELINE: 2

Okay.  I have worked on that today, did more manual tests, and applied
this fix down to 13.  The cherry-pick from 14 to 13 only required a
s/$master/$primary/ in the test, which was straight-forward.  Your
patch for 13 did that though:
-                       if (starttli != entry->tli)
+                       if (!XLogRecPtrIsInvalid(entry->begin))

So it would have caused a failure with parent TLIs that have a correct
begin location, but we expect the opposite.  The patch for 14/HEAD had
that right.
--
Michael

  7. Re: pg_veryfybackup can fail with a valid backup for TLI > 1

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-08-23T07:09:04Z

    At Mon, 23 Aug 2021 15:46:37 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Fri, Aug 20, 2021 at 04:47:15PM +0900, Kyotaro Horiguchi wrote:
> > Yes, backup_label looks correct.
> > 
> > backup_label (extract):
> > START WAL LOCATION: 0/5000028 (file 000000020000000000000005)
> > CHECKPOINT LOCATION: 0/5000060
> > START TIMELINE: 2
> 
> Okay.  I have worked on that today, did more manual tests, and applied
> this fix down to 13.  The cherry-pick from 14 to 13 only required a
> s/$master/$primary/ in the test, which was straight-forward.  Your
> patch for 13 did that though:
> -                       if (starttli != entry->tli)
> +                       if (!XLogRecPtrIsInvalid(entry->begin))
> 
> So it would have caused a failure with parent TLIs that have a correct
> begin location, but we expect the opposite.  The patch for 14/HEAD had
> that right.

Mmm. Sorry for the silly mistake, and thanks for commiting this!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center