cleanup patches for incremental backup

Hello again,

I have now committed 0001.

I got some off-list review of 0004; specifically, Michael Paquier said
it looked OK, and Tom Lane found another bug. So I've added a fix for
that in what's now 0003.

Here's v2. I plan to commit the rest of this fairly soon if there are
no comments.

...Robert

On Tue, Jan 9, 2024 at 1:18 PM Robert Haas <robertmhaas@gmail.com> wrote:

Here's v2. I plan to commit the rest of this fairly soon if there are
no comments.

Done, with a minor adjustment to 0003.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,
Thank you for developing the new tool. I have attached a patch that corrects the spelling of the --individual option in the SGML file.

Regards,
Noriyoshi Shinoda

-----Original Message-----
From: Robert Haas <robertmhaas@gmail.com>
Sent: Friday, January 12, 2024 3:13 AM
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Jakub Wartak <jakub.wartak@enterprisedb.com>
Subject: Re: cleanup patches for incremental backup

On Tue, Jan 9, 2024 at 1:18 PM Robert Haas <robertmhaas@gmail.com> wrote:

Here's v2. I plan to commit the rest of this fairly soon if there are
no comments.

Done, with a minor adjustment to 0003.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Jan 11, 2024 at 8:00 PM Shinoda, Noriyoshi (HPE Services Japan
- FSIP) <noriyoshi.shinoda@hpe.com> wrote:

Thank you for developing the new tool. I have attached a patch that corrects the spelling of the --individual option in the SGML file.

Thanks, committed.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hello Robert,

12.01.2024 17:50, Robert Haas wrote:

On Thu, Jan 11, 2024 at 8:00 PM Shinoda, Noriyoshi (HPE Services Japan
- FSIP) <noriyoshi.shinoda@hpe.com> wrote:

Thank you for developing the new tool. I have attached a patch that corrects the spelling of the --individual option in the SGML file.

Thanks, committed.

I've found one more typo in the sgml:
summarized_pid
And one in a comment:
sumamry

A trivial fix is attached.

Best regards,
Alexander

On Sat, Jan 13, 2024 at 1:00 PM Alexander Lakhin <exclusion@gmail.com> wrote:

I've found one more typo in the sgml:
summarized_pid
And one in a comment:
sumamry

A trivial fix is attached.

Thanks, committed.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, 15 Jan 2024 at 17:58, Robert Haas <robertmhaas@gmail.com> wrote:

On Sat, Jan 13, 2024 at 1:00 PM Alexander Lakhin <exclusion@gmail.com> wrote:

I've found one more typo in the sgml:
summarized_pid
And one in a comment:
sumamry

A trivial fix is attached.

Thanks, committed.

Off-list I was notified that the new WAL summarizer process was not
yet added to the glossary, so PFA a patch that does that.
In passing, it also adds "incremental backup" to the glossary, and
updates the documented types of backends in monitoring.sgml with the
new backend type, too.

Kind regards,

Matthias van de Meent.

On Mon, Jan 15, 2024 at 3:31 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:

Off-list I was notified that the new WAL summarizer process was not
yet added to the glossary, so PFA a patch that does that.
In passing, it also adds "incremental backup" to the glossary, and
updates the documented types of backends in monitoring.sgml with the
new backend type, too.

I wonder if it's possible that you sent the wrong version of this
patch, because:

(1) The docs don't build with this applied. I'm not sure if it's the
only problem, but <glossterm linkend="glossary-db-cluster" is missing
the closing >.

(2) The changes to monitoring.sgml contain an unrelated change, about
pg_stat_all_indexes.idx_scan.

Also, I think the "For more information, see <xref linkend="whatever"
/> bit should have a period after the markup tag, as we seem to do in
other cases.

One other thought is that the incremental backup only replaces
relation files with incremental files, and it never does anything
about FSM files. So the statement that it only contains data that was
potentially changed isn't quite correct. It might be better to phrase
it the other way around i.e. it is like a full backup, except that
some files can be replaced by incremental files which omit blocks to
which no WAL-logged changes have been made.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, 16 Jan 2024 at 16:39, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Jan 15, 2024 at 3:31 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:

Off-list I was notified that the new WAL summarizer process was not
yet added to the glossary, so PFA a patch that does that.
In passing, it also adds "incremental backup" to the glossary, and
updates the documented types of backends in monitoring.sgml with the
new backend type, too.

I wonder if it's possible that you sent the wrong version of this
patch, because:

(1) The docs don't build with this applied. I'm not sure if it's the
only problem, but <glossterm linkend="glossary-db-cluster" is missing
the closing >.

That's my mistake, I didn't check install-world yet due to unrelated
issues building the docs. I've since sorted out these issues (this was
a good stick to get that done), so this issue is fixed in the attached
patch.

(2) The changes to monitoring.sgml contain an unrelated change, about
pg_stat_all_indexes.idx_scan.

Thanks for noticing, fixed in attached.

Also, I think the "For more information, see <xref linkend="whatever"
/> bit should have a period after the markup tag, as we seem to do in
other cases.

Fixed.

One other thought is that the incremental backup only replaces
relation files with incremental files, and it never does anything
about FSM files. So the statement that it only contains data that was
potentially changed isn't quite correct. It might be better to phrase
it the other way around i.e. it is like a full backup, except that
some files can be replaced by incremental files which omit blocks to
which no WAL-logged changes have been made.

How about the attached?

Kind regards,

Matthias van de Meent

On Tue, Jan 16, 2024 at 3:22 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:

One other thought is that the incremental backup only replaces
relation files with incremental files, and it never does anything
about FSM files. So the statement that it only contains data that was
potentially changed isn't quite correct. It might be better to phrase
it the other way around i.e. it is like a full backup, except that
some files can be replaced by incremental files which omit blocks to
which no WAL-logged changes have been made.

How about the attached?

I like the direction.

+     A special <glossterm linkend="glossary-basebackup">base backup</glossterm>
+     that for some WAL-logged relations only contains the pages that were
+     modified since a previous backup, as opposed to the full relation data of
+     normal base backups. Like base backups, it is generated by the tool
+     <xref linkend="app-pgbasebackup"/>.

Could we say "that for some files may contain only those pages that
were modified since a previous backup, as opposed to the full contents
of every file"? My thoughts are (1) there's no hard guarantee that an
incremental backup will replace even one file with an incremental
file, although in practice it is probably almost always going to
happen and (2) pg_combinebackup would actually be totally fine with
any file at all being sent incrementally; it's only that the server
isn't smart enough to figure out how to do this with e.g. SLRU data
right now.

+     To restore incremental backups the tool <xref
linkend="app-pgcombinebackup"/>
+     is used, which combines the incremental backups with a base backup and
+     <glossterm linkend="glossary-wal">WAL</glossterm> to restore a
+     <glossterm linkend="glossary-db-cluster">database cluster</glossterm> to
+     a consistent state.

I wondered if this needed to be clearer that the chain of backups
could have length > 2. But on further reflection, I think it's fine,
unless you feel otherwise.

The rest LGTM.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, 16 Jan 2024 at 21:49, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Jan 16, 2024 at 3:22 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
+     A special <glossterm linkend="glossary-basebackup">base backup</glossterm>
+     that for some WAL-logged relations only contains the pages that were
+     modified since a previous backup, as opposed to the full relation data of
+     normal base backups. Like base backups, it is generated by the tool
+     <xref linkend="app-pgbasebackup"/>.

Could we say "that for some files may contain only those pages that
were modified since a previous backup, as opposed to the full contents
of every file"?

Sure, added in attached.

+     To restore incremental backups the tool <pgcombinebackup>
+     is used, which combines the incremental backups with a base backup and
+ [...]
I wondered if this needed to be clearer that the chain of backups
could have length > 2. But on further reflection, I think it's fine,
unless you feel otherwise.

I removed "the" from the phrasing "the incremental backups", which
makes it a bit less restricted.

The rest LGTM.

In the latest patch I also fixed the casing of "Incremental Backup" to
"... backup", to be in line with most other multi-word items.

Thanks!

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Wed, Jan 17, 2024 at 1:42 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:

Sure, added in attached.

I think this mostly looks good now but I just realized that I think
this needs rephrasing:

+     To restore incremental backups the tool <xref
linkend="app-pgcombinebackup"/>
+     is used, which combines incremental backups with a base backup and
+     <glossterm linkend="glossary-wal">WAL</glossterm> to restore a
+     <glossterm linkend="glossary-db-cluster">database cluster</glossterm> to
+     a consistent state.

The way this is worded, at least to me, it makes it sound like
pg_combinebackup is going to do the WAL recovery for you, which it
isn't. Maybe:

To restore incremental backups the tool <xref
linkend="app-pgcombinebackup"/> is used, which combines incremental
backups with a base backup. Afterwards, recovery can use <glossterm
linkend="glossary-wal">WAL</glossterm> to bring the <glossterm
linkend="glossary-db-cluster">database cluster</glossterm> to a
consistent state.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, 17 Jan 2024 at 21:10, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Jan 17, 2024 at 1:42 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:

Sure, added in attached.

I think this mostly looks good now but I just realized that I think
this needs rephrasing:

+     To restore incremental backups the tool <xref
linkend="app-pgcombinebackup"/>
+     is used, which combines incremental backups with a base backup and
+     <glossterm linkend="glossary-wal">WAL</glossterm> to restore a
+     <glossterm linkend="glossary-db-cluster">database cluster</glossterm> to
+     a consistent state.

The way this is worded, at least to me, it makes it sound like
pg_combinebackup is going to do the WAL recovery for you, which it
isn't. Maybe:

To restore incremental backups the tool <xref
linkend="app-pgcombinebackup"/> is used, which combines incremental
backups with a base backup. Afterwards, recovery can use <glossterm
linkend="glossary-wal">WAL</glossterm> to bring the <glossterm
linkend="glossary-db-cluster">database cluster</glossterm> to a
consistent state.

Sure, that's fine with me.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Thu, Jan 18, 2024 at 4:50 AM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:

Sure, that's fine with me.

OK, committed that way.

--
Robert Haas
EDB: http://www.enterprisedb.com

I'm seeing some recent buildfarm failures for pg_walsummary:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sungazer&amp;dt=2024-01-14%2006%3A21%3A58
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=idiacanthus&amp;dt=2024-01-17%2021%3A10%3A36
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=serinus&amp;dt=2024-01-20%2018%3A58%3A49
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=taipan&amp;dt=2024-01-23%2002%3A46%3A57
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=serinus&amp;dt=2024-01-23%2020%3A23%3A36

The signature looks nearly identical in each:

# Failed test 'WAL summary file exists'
# at t/002_blocks.pl line 79.

# Failed test 'stdout shows block 0 modified'
# at t/002_blocks.pl line 85.
# ''
# doesn't match '(?^m:FORK main: block 0$)'

I haven't been able to reproduce the issue on my machine, and I haven't
figured out precisely what is happening yet, but I wanted to make sure
there is awareness.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Jan 24, 2024 at 12:08 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:

I'm seeing some recent buildfarm failures for pg_walsummary:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sungazer&amp;dt=2024-01-14%2006%3A21%3A58
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=idiacanthus&amp;dt=2024-01-17%2021%3A10%3A36
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=serinus&amp;dt=2024-01-20%2018%3A58%3A49
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=taipan&amp;dt=2024-01-23%2002%3A46%3A57
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=serinus&amp;dt=2024-01-23%2020%3A23%3A36

The signature looks nearly identical in each:

# Failed test 'WAL summary file exists'
# at t/002_blocks.pl line 79.

# Failed test 'stdout shows block 0 modified'
# at t/002_blocks.pl line 85.
# ''
# doesn't match '(?^m:FORK main: block 0$)'

I haven't been able to reproduce the issue on my machine, and I haven't
figured out precisely what is happening yet, but I wanted to make sure
there is awareness.

This is weird. There's a little more detail in the log file,
regress_log_002_blocks, e.g. from the first failure you linked:

[11:18:20.683](96.787s) # before insert, summarized TLI 1 through 0/14E09D0
[11:18:21.188](0.505s) # after insert, summarized TLI 1 through 0/14E0D08
[11:18:21.326](0.138s) # examining summary for TLI 1 from 0/14E0D08 to 0/155BAF0
# 1
...
[11:18:21.349](0.000s) # got: 'pg_walsummary: error: could
not open file "/home/nm/farm/gcc64/HEAD/pgsql.build/src/bin/pg_walsummary/tmp_check/t_002_blocks_node1_data/pgdata/pg_wal/summaries/0000000100000000014E0D0800000000155BAF0
# 1.summary": No such file or directory'

The "examining summary" line is generated based on the output of
pg_available_wal_summaries(). The way that works is that the server
calls readdir(), disassembles the filename into a TLI and two LSNs,
and returns the result. Then, a fraction of a second later, the test
script reassembles those components into a filename and finds the file
missing. If the logic to translate between filenames and TLIs & LSNs
were incorrect, the test would fail consistently. So the only
explanation that seems to fit the facts is the file disappearing out
from under us. But that really shouldn't happen. We do have code to
remove such files in MaybeRemoveOldWalSummaries(), but it's only
supposed to be nuking files more than 10 days old.

So I don't really have a theory here as to what could be happening. :-(

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Jan 24, 2024 at 12:46:16PM -0500, Robert Haas wrote:

The "examining summary" line is generated based on the output of
pg_available_wal_summaries(). The way that works is that the server
calls readdir(), disassembles the filename into a TLI and two LSNs,
and returns the result. Then, a fraction of a second later, the test
script reassembles those components into a filename and finds the file
missing. If the logic to translate between filenames and TLIs & LSNs
were incorrect, the test would fail consistently. So the only
explanation that seems to fit the facts is the file disappearing out
from under us. But that really shouldn't happen. We do have code to
remove such files in MaybeRemoveOldWalSummaries(), but it's only
supposed to be nuking files more than 10 days old.

So I don't really have a theory here as to what could be happening. :-(

There might be an overflow risk in the cutoff time calculation, but I doubt
that's the root cause of these failures:

/*
* Files should only be removed if the last modification time precedes the
* cutoff time we compute here.
*/
cutoff_time = time(NULL) - 60 * wal_summary_keep_time;

Otherwise, I think we'll probably need to add some additional logging to
figure out what is happening...

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Jan 24, 2024 at 1:05 PM Nathan Bossart <nathandbossart@gmail.com> wrote:

There might be an overflow risk in the cutoff time calculation, but I doubt
that's the root cause of these failures:

/*
* Files should only be removed if the last modification time precedes the
* cutoff time we compute here.
*/
cutoff_time = time(NULL) - 60 * wal_summary_keep_time;

Otherwise, I think we'll probably need to add some additional logging to
figure out what is happening...

Where, though? I suppose we could:

1. Change the server code so that it logs each WAL summary file
removed at a log level high enough to show up in the test logs.

2. Change the TAP test so that it prints out readdir(WAL summary
directory) at various points in the test.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Jan 24, 2024 at 02:08:08PM -0500, Robert Haas wrote:

On Wed, Jan 24, 2024 at 1:05 PM Nathan Bossart <nathandbossart@gmail.com> wrote:

Otherwise, I think we'll probably need to add some additional logging to
figure out what is happening...

Where, though? I suppose we could:

1. Change the server code so that it logs each WAL summary file
removed at a log level high enough to show up in the test logs.

2. Change the TAP test so that it prints out readdir(WAL summary
directory) at various points in the test.

That seems like a reasonable starting point. Even if it doesn't help
determine the root cause, it should at least help rule out concurrent
summary removal.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com