EnterpriseDB / barman

Barman - Backup and Recovery Manager for PostgreSQL
https://www.pgbarman.org/
GNU General Public License v3.0
2.07k stars 191 forks source link

create new recovery executor that uses `pg_combinebackup` #955

Closed andremagui closed 2 months ago

andremagui commented 3 months ago

pg_combinebackup utility is coming to Postgresql 17 with the introduction of incremental backup to reconstruct a synthetic full backup from a sequence of incremental backups and the previous full backup.

This PR:

Note: we are reusing the --recovery-staging-path config options to point the place where pg_combinebackup should create the synthetic backup in the Barman host. Originally that config option was used only to point the path in the Postgres host where a backup should be decompressed.

References: BAR-167 & BAR-168

barthisrael commented 2 months ago

I sent a commit with a suggestion for test_cli unit tests (basically combining 3 different methods into a single one which uses pytest.parametrize).

barthisrael commented 2 months ago

I submitted a couple of fixes to the unit tests along with a final round of review/suggestions. After the suggestions are applied, I think we can work on making the commit log cleaner and proceed with merging.

barthisrael commented 2 months ago

(FWIW I was able to start all "succeeded" Postgres instances from the tests shared through the previous comment).

gcalacoci commented 2 months ago

I promised @barthisrael to look at this before leaving for PTO 😄 , and I always keep my promises!

Important things first: I've executed the unit tests, set up a pg17 and executed a local backup and restore without tablespaces, I didn't go into deep testing like @barthisrael has done.

I like this PR and to me, it is in good shape, just need to apply @martinmarques suggestion and squash in 2 or 3 commits instead of 7, once those things are done, consider this comment as my approval!

barthisrael commented 2 months ago

Thanks for also taking a look at this @gcalacoci -- and enjoy your Vacations 😆 !

I agree with applying the suggestion from Martin, but I guess we should have those 7 commits, though. This PR is big, and thinking of it now we could've split this into at least 3 cards:

The fourth commit is about fixing tests which got broken by the commits in this PR, and the fifth about introducing unit tests to cover the changes from this PR.

The 6th is an "unrelated change" which improves the unit test suite, and the 7th is an "unrelated change" which was introduced by Actions.

To sum up my view of this: yes, I'm also bothered about the amount of commits, but I guess that's a consequence of the big task which was not properly split by us.

With that in mind I'd say to proceed like this:

  1. Apply the suggestion from @martinmarques and squash that into the third commit (321ccce4d5ef25d7241c879d1c1cfa2ca89f9560);
  2. Remove the changes related with command_wrappers.py file from the third commit (321ccce4d5ef25d7241c879d1c1cfa2ca89f9560), and "squash" those changes into the first commit (d6985887677dbb51bbb45b42c6ce309ec1a966fa). What I mean here is: the first commit is in charge of adding the pg_combinebackup command wrapper, so the suggestion that I made in regards to its docstring should be part of that commit, not of the one which adds a recovery executor;
  3. Reword a bit the second commit (d2f07ae7b5a1240f93aa09ef6a553139d994b787). It says we are modifying the recover method to use --recovery-staging-path, but in fact we are only making sure the value is set when recovering an incremental backup, and aborting execution if that's not the case.

Makes sense?