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

Hello Robert,

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

Thanks for your review!

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

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.

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

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

> 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

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.