search

pgstef / check_pgbackrest

pgBackRest backup check plugin for Nagios
PostgreSQL License
35 stars 14 forks source link

Warning issued if pgbackrest.conf contains repo1-retention-archive-type=diff #10

Closed np422 closed 4 years ago

np422 commented 4 years ago

Hello,

We have configured pgbackrest with the repo1-retention-archive-type=diff option.

But when check_pgbackrest is used to check -s archives ... it will print a warning after the first differential backup is done as pbgackrest then removes the archives between the full backup and the latest diff backup, as instructed by the retention-archive-type option in pgbackrest.conf.

How can this warning be avoided as this is the expected behaviour with our configuration of pgbackrest?

pgstef commented 4 years ago

Hi,

This isn't currently supported in check_pgbackrest. The archives service tends to check all of them. With your configuration, you lose the possibility to achieve PITR between your full and diff backups.

This behaviour is also not recommended by pgBackRest itself:

It is recommended that this setting not be changed from the default 
which will only expire WAL in conjunction with expiring full backups.

That's basically why you get a WARNING and not a CRITICAL alert.

I would have to take a look at the new time-based retention in pgBackRest and see if we have to add more options here to know how difficult it would be to support your specific feature request.

maglub commented 4 years ago

Hi!

Disclosure: I am affiliated with np422, we are working together.

Firstly, thanks for answering so fast!

I have been thinking a bit; Even if that setting is not recommended, it is still a valid configuration of pgBackRest. And there are some compelling situations where this might be a more than valid configuration. For example, if we ensure that we have all the WAL files in a different location, or perhaps just don't care about PITR between the last full backup and the last incremental.

Is there any way we can help in extending the check script?

pgstef commented 4 years ago

Well, I'm not questioning your configuration. Just pointing out that it breaks the PITR possibility. That's why the archives check service is made for. And raising a WARNING in this case seems reasonable from this point of view.

The retention parameters haven't been implemented for that service. It would be a big code change and I'm not sure it would worth it. I will have a look soon at the new time-based retention (https://github.com/pgbackrest/pgbackrest/commit/cdebfb09e01e64d6acfe650345a56d2b1805374e) and see if it has an impact for check_pgbackrest or not. If I have to rework the retention settings, I might consider to touch the archives service too.

But it's more a "philosophy" issue. It's against the design goal of this service : PITR capability.

maglub commented 4 years ago

Not to be a stick in your behind, but PITR capability from a certain point in time (i.e last full backup or last incremental) is probably more what I have in mind when I design a backup strategy.

But I do get your point.

pgstef commented 4 years ago

Well, with all my respect, you design your strategy the way you want. That's not the point here. Your feature request would be a big code change and against the original philosophy of this service. There are missing WAL archives in the repo (for any "good" reason or not), so a warning is raised.

Except if the support of the pgbackrest commit mentioned above requires a big change, and allows to support your request too, this feature won't be included.

Regards