From dcaad6013f3dbc6067b13c4f796bfde831d6612f Mon Sep 17 00:00:00 2001 From: Daria Lepikhova Date: Mon, 28 Sep 2026 16:48:06 +0200 Subject: [PATCH v2] Report correct size estimate for incremental backups The estimate behind pg_basebackup --progress and pg_stat_progress_basebackup was computed by a size-only pass over the directory tree that was never told the backup is incremental, so every file was counted at its full size. A finished incremental backup could therefore sit at a few percent, even though backup_type already reported it as incremental. Pass the IncrementalBackupInfo to that pass, so that files which will be sent incrementally are counted at the size they will actually take. The size reported for a tablespace is now the amount of data expected to be sent rather than the size of the directory, so update the protocol documentation. Also free the path that GetFileBackupMethod() builds to look up an incremental file, which was never freed and is now built twice per file. Add a TAP test comparing the totals reported for a full and an incremental backup. --- doc/src/sgml/protocol.sgml | 13 ++- doc/src/sgml/ref/pg_basebackup.sgml | 4 +- src/backend/backup/basebackup.c | 11 +-- src/backend/backup/basebackup_incremental.c | 5 +- src/bin/pg_basebackup/meson.build | 1 + .../t/012_incremental_progress.pl | 84 +++++++++++++++++++ src/include/backup/basebackup_sink.h | 6 +- 7 files changed, 110 insertions(+), 14 deletions(-) create mode 100644 src/bin/pg_basebackup/t/012_incremental_progress.pl diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index 8ced69f283b..4f7f7b7d4e2 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -3200,7 +3200,9 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;" before the transfer is even started, and might as such have a negative impact on the performance. In particular, it might take longer before the first data - is streamed. Since the database files can change during the backup, + is streamed. For an incremental backup, each file is counted at the + size it will occupy in the backup rather than its size on disk. + Since the database files can change during the backup, the size is only approximate and might both grow and shrink between the time of approximation and the sending of the actual files. The default is false. @@ -3425,8 +3427,13 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;" size (int8) - The approximate size of the tablespace, in kilobytes (1024 bytes), - if progress report has been requested; otherwise it's null. + The approximate amount of data that will be sent for the + tablespace, in kilobytes (1024 bytes), if progress report has been + requested; otherwise it's null. For an incremental backup this can + be much smaller than the size of the tablespace, since a relation + file that has not changed since the prior backup, or has changed + only in part, is replaced by a smaller incremental file containing + just the blocks that have changed. diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml index 3117968d125..b4a0faccf41 100644 --- a/doc/src/sgml/ref/pg_basebackup.sgml +++ b/doc/src/sgml/ref/pg_basebackup.sgml @@ -725,8 +725,8 @@ PostgreSQL documentation always being NULL. - Without this option, the backup will start by enumerating - the size of the entire database, and then go back and send + Without this option, the backup will start by estimating the total + amount of data that will be streamed, and then go back and send the actual contents. This may make the backup take slightly longer, and in particular it will take longer before the first data is sent. This option is useful to avoid such estimation diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c index e3c04ecd810..d674fb27543 100644 --- a/src/backend/backup/basebackup.c +++ b/src/backend/backup/basebackup.c @@ -302,8 +302,8 @@ perform_base_backup(basebackup_options *opt, bbsink *sink, state.tablespaces = lappend(state.tablespaces, newti); /* - * Calculate the total backup size by summing up the size of each - * tablespace + * Calculate the total backup size by summing up the amount of data + * that will be sent for each tablespace */ if (opt->progress) { @@ -315,10 +315,10 @@ perform_base_backup(basebackup_options *opt, bbsink *sink, if (tmp->path == NULL) tmp->size = sendDir(sink, ".", 1, true, state.tablespaces, - true, NULL, InvalidOid, NULL); + true, NULL, InvalidOid, ib); else tmp->size = sendTablespace(sink, tmp->path, tmp->oid, true, - NULL, NULL); + NULL, ib); state.bytes_total += tmp->size; } state.bytes_total_is_valid = true; @@ -1183,7 +1183,8 @@ sendTablespace(bbsink *sink, char *path, Oid spcoid, bool sizeonly, /* * Include all files from the given directory in the output tar stream. If * 'sizeonly' is true, we just calculate a total length and return it, without - * actually sending anything. + * actually sending anything. A file that will be sent incrementally is + * counted at the size of the incremental file. * * Omit any directory in the tablespaces list, to avoid backing up * tablespaces twice when they were created inside PGDATA. diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c index 27fc42a2a43..ac4944c06a0 100644 --- a/src/backend/backup/basebackup_incremental.c +++ b/src/backend/backup/basebackup_incremental.c @@ -723,10 +723,13 @@ GetFileBackupMethod(IncrementalBackupInfo *ib, const char *path, if (backup_file_lookup(ib->manifest_files, path) == NULL) { char *ipath; + bool found; ipath = GetIncrementalFilePath(dboid, spcoid, relfilenumber, forknum, segno); - if (backup_file_lookup(ib->manifest_files, ipath) == NULL) + found = backup_file_lookup(ib->manifest_files, ipath) != NULL; + pfree(ipath); + if (!found) return BACK_UP_FILE_FULLY; } diff --git a/src/bin/pg_basebackup/meson.build b/src/bin/pg_basebackup/meson.build index d70ce5786a2..2d3b22ff06e 100644 --- a/src/bin/pg_basebackup/meson.build +++ b/src/bin/pg_basebackup/meson.build @@ -100,6 +100,7 @@ tests += { 'tests': [ 't/010_pg_basebackup.pl', 't/011_in_place_tablespace.pl', + 't/012_incremental_progress.pl', 't/020_pg_receivewal.pl', 't/030_pg_recvlogical.pl', 't/040_pg_createsubscriber.pl', diff --git a/src/bin/pg_basebackup/t/012_incremental_progress.pl b/src/bin/pg_basebackup/t/012_incremental_progress.pl new file mode 100644 index 00000000000..70a385ef324 --- /dev/null +++ b/src/bin/pg_basebackup/t/012_incremental_progress.pl @@ -0,0 +1,84 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Verify that the backup size estimate reported for an incremental backup +# reflects the amount of data that will actually be sent, rather than the size +# of a full backup. + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('primary'); +$node->init(allows_streaming => 1); +$node->append_conf('postgresql.conf', 'summarize_wal = on'); +$node->append_conf('postgresql.conf', 'autovacuum = off'); +$node->start; + +# Enough data that a full backup and an incremental one differ clearly in +# size, without writing more than necessary. +$node->safe_psql('postgres', < $node->host, + '--port' => $node->port, + '--pgdata' => $path, + '--no-sync', + '--progress', + '--checkpoint' => 'fast', + @extra + ], + '>' => \$stdout, + '2>' => \$stderr); + ok($result, "backup into $path succeeded") + or die "pg_basebackup failed: $stderr"; + + # Progress lines are separated by carriage returns and look like + # " 6519/346720 kB (1%), 1/1 tablespace". + my @lines = grep { /kB/ } split(/[\r\n]+/, $stderr); + ok(@lines > 0, "progress was reported for $path") + or die "no progress output for $path"; + + my ($total) = $lines[-1] =~ m{\d+/(\d+) kB}; + defined($total) + or die "could not parse progress line for $path: $lines[-1]"; + return $total; +} + +my $backup_dir = $node->backup_dir; +my $full_total = reported_total("$backup_dir/full"); + +# A small change, so that the incremental backup has very little to send. +$node->safe_psql('postgres', < "$backup_dir/full/backup_manifest"); + +note "full backup reported a total of $full_total kB"; +note "incremental backup reported a total of $incr_total kB"; + +# An incremental backup sends only a fraction of the cluster, so an estimate +# that still describes a full backup is far too large. +cmp_ok($incr_total, '<', $full_total / 2, + 'incremental backup estimate is much smaller than a full backup estimate' +); + +done_testing(); diff --git a/src/include/backup/basebackup_sink.h b/src/include/backup/basebackup_sink.h index 96fa2c4eaba..8fdc894851d 100644 --- a/src/include/backup/basebackup_sink.h +++ b/src/include/backup/basebackup_sink.h @@ -51,10 +51,10 @@ typedef struct bbsink_ops bbsink_ops; * 'tablespace_num' is the index of the current tablespace within the list * stored in 'tablespaces'. * - * 'bytes_done' is the number of bytes read so far from $PGDATA. + * 'bytes_done' is the number of bytes sent so far. * - * 'bytes_total' is the total number of bytes estimated to be present in - * $PGDATA, if we have estimated this. + * 'bytes_total' is the total number of bytes estimated to be sent, if we + * have estimated this. * * 'bytes_total_is_valid' is true if and only if a proper estimate has been * stored into 'bytes_total'. base-commit: 2906f0d5d7e2d3289f41b98115836c0bc15ab43e -- 2.50.1 (Apple Git-155)