Subject: Add -k/--link option to pg_combinebackup

From: Israel Barth Rubio <[email-masked]>
Date: Wed, 15 Jan 2025 14:42:23 -0300

Hello all,

With the current implementation of pg_combinebackup, we have a few
copy methods: --clone, --copy and --copy-file-range. By using either of
them, it implicates an actual file copy in the file system, i.e. among
other things, disk usage.

While discussing with some people, e.g. Robert Haas and Martín Marqués,
about possible ways to improve pg_combinebackup performance and/or
reduce disk usage taken by the synthetic backup, there was a thought
of using hard links.

I'm submitting a patch in this thread which introduces the -k/--link
option to pg_combinebackup, making it similar to the options exposed
by pg_upgrade. The patch reuses the same code flow that already exists
in pg_combinebackup, and whenever the tool judges a file should be
copied from either of the input backups to the synthetic backup, if the
link mode was chosen, a hard link is created instead of performing a
copy.

Depending on the pattern of modification of PGDATA files between
backups (based on the workload), the user might face improved
performance of pg_combinebackup as well as lower disk usage by the
synthetic backup.

That enhancement comes at a cost: the input backups may be invalidated
if the user modified the shared files from the synthetic backup, or if
the user starts the cluster from the synthetic backup. With that in
mind, a warning has been added to pg_combinebackup, as well as to the
docs, recommending that the user moves the synthetic backup to another
file system or machine before making use of it.

I've run the existing test suite with --link option and it worked fine,
except for an issue when running pg_verifybackup in t/003_timeline.pl.
The issue was expected given the description shared above. I modified
the tests to behave slightly differently depending on the copy method
selected by the runner.

Besides that, you can find below the outcomes of manual testing:

* Create some tables:

postgres=# CREATE TABLE test_1 AS SELECT generate_series(1, 1000000);
SELECT 1000000
postgres=# CREATE TABLE test_2 AS SELECT generate_series(1, 1000000);
SELECT 1000000
postgres=# CREATE TABLE test_3 AS SELECT generate_series(1, 1000000);
SELECT 1000000

* Check their OID:

postgres=# SELECT oid, relname FROM pg_class WHERE relname LIKE 'test_%';
  oid  | relname
-------+---------
 16388 | test_1
 16391 | test_2
 16394 | test_3
(3 rows)

* Enable the WAL summarizer:

postgres=# ALTER SYSTEM SET summarize_wal TO on;
ALTER SYSTEM
postgres=# SELECT pg_reload_conf();
 pg_reload_conf
----------------
 t
(1 row)

* Take a full backup:

$ pg_basebackup -d 'dbname=postgres' -D ~/monday -c fast -Xn
NOTICE:  WAL archiving is not enabled; you must ensure that all required
WAL segments are copied through other means to complete the backup

* Perform an update that will touch the whole file:

postgres=# UPDATE test_2 SET generate_series = generate_series + 1;
UPDATE 1000000

* Perform an update that will touch a single page:

postgres=# UPDATE test_3 SET generate_series = generate_series + 1 WHERE
generate_series = 1;
UPDATE 1

* Take an incremental backup:

$ pg_basebackup -d 'dbname=postgres' -D ~/tuesday -c fast -Xn --incremental
~/monday/backup_manifest
NOTICE:  WAL archiving is not enabled; you must ensure that all required
WAL segments are copied through other means to complete the backup

* Check a dry run of pg_combinebackup, focusing on the OIDs of the
  tables:

$ pg_combinebackup -d -n --link -o ~/tuesday_full ~/monday ~/tuesday/ 2>&1
| grep -P '16388|16391|16394'
pg_combinebackup: would copy "/home/vagrant/monday/base/5/16388" to
"/home/vagrant/tuesday_full/base/5/16388" using strategy link
pg_combinebackup: would copy "/home/vagrant/tuesday//base/5/16391" to
"/home/vagrant/tuesday_full/base/5/16391" using strategy link
pg_combinebackup: would reconstruct
"/home/vagrant/tuesday_full/base/5/16394" (4480 blocks, checksum CRC32C)
pg_combinebackup: reconstruction plan:
0:/home/vagrant/tuesday//base/5/INCREMENTAL.16394@8192
1-4423:/home/vagrant/monday/base/5/16394@36233216
4424:/home/vagrant/tuesday//base/5/INCREMENTAL.16394@16384
4425-4479:/home/vagrant/monday/base/5/16394@36691968
pg_combinebackup: would have read 4478 blocks from
"/home/vagrant/monday/base/5/16394"
pg_combinebackup: would have read 2 blocks from
"/home/vagrant/tuesday//base/5/INCREMENTAL.16394"
pg_combinebackup: would copy "/home/vagrant/monday/base/5/16388_vm" to
"/home/vagrant/tuesday_full/base/5/16388_vm" using strategy link
pg_combinebackup: would copy "/home/vagrant/tuesday//base/5/16388_fsm" to
"/home/vagrant/tuesday_full/base/5/16388_fsm" using strategy link
pg_combinebackup: would copy "/home/vagrant/tuesday//base/5/16391_vm" to
"/home/vagrant/tuesday_full/base/5/16391_vm" using strategy link
pg_combinebackup: would copy "/home/vagrant/tuesday//base/5/16391_fsm" to
"/home/vagrant/tuesday_full/base/5/16391_fsm" using strategy link
pg_combinebackup: would copy "/home/vagrant/monday/base/5/16394_vm" to
"/home/vagrant/tuesday_full/base/5/16394_vm" using strategy link
pg_combinebackup: would copy "/home/vagrant/tuesday//base/5/16394_fsm" to
"/home/vagrant/tuesday_full/base/5/16394_fsm" using strategy link

* Create a synthetic backup:
$ pg_combinebackup --link -o ~/tuesday_full ~/monday ~/tuesday
pg_combinebackup: warning: Modifying files or starting the cluster from the
synthetic backup might invalidate one or more of the input backups
pg_combinebackup: hint: It is recommended to move the synthetic backup to
another file system or machine before performing changes and/or starting
the cluster

* Check files with link counter set to 1:

$ find ~/tuesday_full -type f -links 1
/home/vagrant/tuesday_full/backup_label
/home/vagrant/tuesday_full/base/5/1259
/home/vagrant/tuesday_full/base/5/2619
/home/vagrant/tuesday_full/base/5/16394
/home/vagrant/tuesday_full/backup_manifest

* Check files with link counter set to 2, focusing on the OIDs of the
  tables:

$ find ~/tuesday_full -type f -links 2 | grep -P '16388|16391|16394'
/home/vagrant/tuesday_full/base/5/16388
/home/vagrant/tuesday_full/base/5/16391
/home/vagrant/tuesday_full/base/5/16388_vm
/home/vagrant/tuesday_full/base/5/16388_fsm
/home/vagrant/tuesday_full/base/5/16391_vm
/home/vagrant/tuesday_full/base/5/16391_fsm
/home/vagrant/tuesday_full/base/5/16394_vm
/home/vagrant/tuesday_full/base/5/16394_fsm

(There are many other linked files).

---

From: Israel Barth Rubio <[email-masked]>
Date: Wed, 15 Jan 2025 15:54:28 -0300

One concern that I have about this --link mode, which Euler Taveira
also got concerned about: the fact that it can invalidate the original
backups if the user modifies or starts the synthetic backup without
moving it to another file system or machine.

At the moment, pg_combinebackup issues a warning about that problem as
soon as the execution of the tool is about to finish. There is also a
warning in the docs. But some users don't pay proper attention to the
output and/or docs.

I wonder if we have some way, like pg_upgrade does (rename a global
file in the original cluster), so we could increase the protection
against that issue?

---

From: Robert Haas <[email-masked]>
Date: Tue, 4 Feb 2025 15:45:13 -0500

On Wed, Jan 15, 2025 at 12:42 PM Israel Barth Rubio
<[email-masked]> wrote:
> by pg_upgrade.

In general, +1, although I think that the patch needs polishing in a
bunch of areas.

Originally, I thought what we wanted was something like a --in-place
option to pg_combinebackup, allowing the output directory to be the
same as one of the input directories. However, I now think that what
this patch does is likely better, and this patch is a lot simpler to
write than that one would be. With the --in-place option, if I'm
combine backup1 and backup2, I must write either "pg_combinebackup
backup1 backup2 -o backup1" or "pg_combinebackup backup1 backup2 -o
backup2". In either case, I can skip whole-file copies from one of the
two source directories -- whichever one happens to be the output
directory. However, if I write "pg_combinebackup --link backup1
backup2 -o result", I can skip *all* whole-file copies from *every*
input directory, which seems a lot nicer.

Furthermore, if all the input directories are staging directories,
basically copies of original backups stored elsewhere, then the fact
that those directories get trashed is of no concern to me. In fact,
they don't even get trashed if pg_combinebackup is interrupted partway
through, because I can just remove the output directory and recreate
it and everything is fine. With an --in-place option, that would be
trickier.

Regarding the patch itself, I think we need to rethink the test case
changes in particular. As written, the patch won't do any testing of
link mode unless the user sets PG_TEST_PG_COMBINEBACKUP_MODE=--link;
and if they do set that option, then we won't test anything else.
Also, even with that environment variable set, we'll only test --link
mode a bit ... accidentally. The patch doesn't really do anything to
make sure that link mode actually does what it's intended to do. It
only adapts the existing tests not to fail. I think it would be better
to decide that you're not supposed to set
PG_TEST_PG_COMBINEBACKUP_MODE=--link, and then write some new test,
not depending on PG_TEST_PG_COMBINEBACKUP_MODE, that specifically
verifies that link mode behaves as expected.

After all, link mode is noticeably different from the other copy
modes. Those should all produce equivalent results, or fail outright,
we suppose. This produces a different user-visible result, so we
probably ought to test that we get that result. For example, we might
check that certain files end up with a link count of 1 or 2, as
appropriate.

Does link() work on Windows?

I'm not sure what to do about the issue that using --link trashes the
input cluster if you subsequently start the database or otherwise
modify any hard-linked files. Keep in mind that, for command-line
arguments other than the first, these are incremental backups, and you
already can't run postgres on those directories. Only the first input
directory, which is a full backup not an incremental, is a potential
target for an unexpected database start. I'm tentatively inclined to
think we shouldn't modify the input directories and just emit a
warning like:

warning: --link mode was used; any modifications to the output
directory may destructively modify input directories

...but maybe that's not the right idea. Happy to hear other opinions.

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

---

From: Israel Barth Rubio <[email-masked]>
Date: Fri, 7 Feb 2025 23:12:59 -0300

Hello Robert,

> In general, +1, although I think that the patch needs polishing in a

Thanks for your review!

> it and everything is fine. With an --in-place option, that would be

I see! I thought of not reinventing the wheel while writing, so I replayed
in pg_combinebackup the same behavior implemented in pg_upgrade.
And turns out the outcomes sounded convincing.

> check that certain files end up with a link count of 1 or 2, as

That makes sense to me too. I'm submitting a new patch which includes
a new file 010_links.pl. That test takes care of generating full and
incremental,
backups, then combines them with --link mode and ensures that the hard link
count for all files under PGDATA is as expected based on the operations that
were applied in the cluster.

I kept the "support" for PG_TEST_PG_COMBINEBACKUP_MODE=--link
around, just in case someone wants to run that and verify those outcomes
with
the --link mode. At least now we might have a proper test for checking
--link,
which always runs independently of the env variable.

> Does link() work on Windows?

link is a function available only in Linux. The equivalent function in
Windows is
CreateHardLink.

However, in Postgres link works in either of the operating systems because
we
implement a link function, which wraps the call to CreateHardLink. That's
coded
in the file src/port/win32link.c

> directory may destructively modify input directories

The previous patch already had a warning. But I enjoyed your message,
so I slightly changed the warning and the docs in the new version of the
patch.

Best regards,
Israel.