Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Get the correct increment backup #176

Merged
merged 25 commits into from
May 28, 2021
Merged

Get the correct increment backup #176

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
2481dc0
get the correct increment backup
huangfumingyue May 18, 2021
2b2078d
Merge pull request #3 from huangfumingyue/fix-the-increment-backup
huangfumingyue May 18, 2021
6d74b7f
Revert "get the correct increment backup"
huangfumingyue May 18, 2021
48008dc
Merge pull request #4 from huangfumingyue/revert-3-fix-the-increment-…
huangfumingyue May 18, 2021
66f4099
get the correct increment backup
huangfumingyue May 18, 2021
4aa9439
add the regression test of increment backup
huangfumingyue May 24, 2021
84619bc
add the regression test of increment backup
huangfumingyue May 24, 2021
24fd3b4
add the comment
huangfumingyue May 24, 2021
3372846
Add a comment
huangfumingyue May 25, 2021
e911da8
add the regression test of increment backup
huangfumingyue May 25, 2021
b5e24ba
add the regression test of increment backup
huangfumingyue May 25, 2021
dabee15
add the comment
huangfumingyue May 25, 2021
5099504
add the regression test
huangfumingyue May 25, 2021
b813cb9
delete file
huangfumingyue May 25, 2021
6a4007b
add the regression test
huangfumingyue May 25, 2021
7aa3475
Reflected the comment
huangfumingyue May 26, 2021
1435b92
Reflected the comment
huangfumingyue May 26, 2021
e197388
Reflected the comment
huangfumingyue May 26, 2021
cf92d36
Reflected the comment
huangfumingyue May 26, 2021
aa2e6c4
Reflected the comment
huangfumingyue May 26, 2021
482dbb6
Reflected the comment
huangfumingyue May 26, 2021
5b95534
Reflected the comment
huangfumingyue May 26, 2021
17ac111
Regression test command was modified
huangfumingyue May 27, 2021
a23a8ea
Regression test command was modified
huangfumingyue May 27, 2021
622c787
Regression test command was modified
huangfumingyue May 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions backup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ do_backup_database(parray *backup_list, pgBackupOption bkupopt)
pgBackup *prev_backup;

/* find last completed database backup */
prev_backup = catalog_get_last_full_backup(backup_list);
prev_backup = catalog_get_last_data_backup(backup_list);
if (prev_backup == NULL)
{
if (current.full_backup_on_error)
Expand Down Expand Up @@ -185,14 +185,19 @@ do_backup_database(parray *backup_list, pgBackupOption bkupopt)
/*
* To take incremental backup, the file list of the latest validated
* full database backup is needed.
* There is a problem of increment backup.
Copy link
Contributor

@mikecaat mikecaat May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to add problem and solution idea.
So, why don't you change to the following comments?

TODO: fix for issue #154
When a backup list is deleted with rm command or pg_rman's delete command 
with '--force' option, pg_rman can't detect there is a missing piece of backup.
We need the way tracing the backup chains or something else...

@MoonInsung
Is the target which wants to detect something wrong a physical backup file, a backup list, or both?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the target is only detect something wrong a physical backup file.
so I didn't reflect this part 「or pg_rman's delete command
with '--force' option」.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's both.

It is a challenging task,
but it is necessary to check whether the physical backup file is
corrupted and the backup chain list is regular.
It is difficult to write a detailed example here., however, the explanation is omitted from the backup file
because it is essential to check that all backup files are normally stored (including physical corruption).

And, I think that the verification(check) of the backup file chain list is
necessary to ensure that incremental backups and restores operate normally.
For example, if there is a chain list of backup files of
FB(1gen)->IB(1gen-1)->IB(1gen-2)->IB(1gen-3),
If IB(1gen-2) is forcibly deleted,
it is fine until IB (1gen-1), but backup and restore will not be possible after that.
If it doesn't detect this, it could get a meaningless backup, and the wrong restore could work.

Therefore, I think @mikecaat suggestion looks good.

Best regards.
Moon.

Copy link
Contributor

@mikecaat mikecaat May 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@huangfumingyue @MoonInsung
Thanks for your comments!

OK. I understood that there are two problems to be solved.
If so, is it better to separate the issue because (1) and (2) seem to be different problems and need different solutions.

(1) The first is to check files corruption, missing files, and so on only in one backup list.

I couldn't understand this problem clearly because do_validate() already takes charge of checking the file's corruption with CRC and detecting the missing files.

For example, the case is following.

$ rm ~/tmp/backup/20210527/090151/database/base/12405/1247
$ pg_rman restore
(omit)
WARNING: backup file "/home/ikeda/tmp/backup/20210527/090151/database/base/12405/1247" vanished
WARNING: backup "2021-05-27 09:01:51" is corrupted
INFO: restoring database files from the incremental mode backup "2021-05-27 09:01:51"
ERROR: could not open backup file "/home/ikeda/tmp/backup/20210527/090151/database/base/12405/1247": そのようなファイルやディレクトリはありません
(omit)

What am I missing? Is there any corner case?

(2) Second is to check the backup chain is correct. I agree this is not implemented yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear @mikecaat
Thank you.

Sorry. My explanation was insufficient.

In my opinion,
is check whether the backup file is corrupted is when acquiring a new incremental backup.
In other words,
(1) Is all backup files of the generation that try incremental backup normal?
(2) Is the backup file of the chain normal?
I thought it was necessary to check.
Of course, this could maybe be the wrong idea because it's my personal opinion.
That's why I think it needs to be discussed later.

Best regards.
Moon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MoonInsung
Thanks! I understood. I updated the issue to mention the above.

* When a physical backup file is deleted with rm command or pg_rman's '--force' option.
* It cannot find the correct status of a full backup and incremental backup.
* we need to discuss it later to resolve this problem.
* please refer to issue#154 to details.
*/
if (current.backup_mode < BACKUP_MODE_FULL)
{
pgBackup *prev_backup;
uint32 xlogid, xrecoff;

/* find last completed database backup */
prev_backup = catalog_get_last_full_backup(backup_list);
prev_backup = catalog_get_last_data_backup(backup_list);
if (prev_backup == NULL || prev_backup->tli != current.tli)
{
if (current.full_backup_on_error)
Expand Down
7 changes: 3 additions & 4 deletions catalog.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ catalog_get_backup_list(const pgBackupRange *range)
* Find the last completed database full valid backup from the backup list.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a comment so that you can specify that the problem still exists?
Check the already registered issue #154, it will be helpful to add comments on the remaining issues.

pgBackup *
catalog_get_last_full_backup(parray *backup_list)
catalog_get_last_data_backup(parray *backup_list)
{
int i;
pgBackup *backup = NULL;
Expand All @@ -276,9 +276,8 @@ catalog_get_last_full_backup(parray *backup_list)
{
backup = (pgBackup *) parray_get(backup_list, i);

/* Return the first full valid backup. */
if (backup->backup_mode == BACKUP_MODE_FULL &&
backup->status == BACKUP_STATUS_OK)
/* we need completed database backup */
if (backup -> status == BACKUP_STATUS_OK && HAVE_DATABASE(backup))
return backup;
}

Expand Down
16 changes: 12 additions & 4 deletions expected/backup.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,23 @@
0
1
###### BACKUP COMMAND TEST-0005 ######
###### Make sure incremental backup work ######
0
CREATE TABLE
INSERT 0 1000000
0
0
1
###### BACKUP COMMAND TEST-0006 ######
###### full backup with compression ######
0
1
1
###### BACKUP COMMAND TEST-0006 ######
###### BACKUP COMMAND TEST-0007 ######
###### full backup with smooth checkpoint ######
0
1
###### BACKUP COMMAND TEST-0007 ######
###### BACKUP COMMAND TEST-0008 ######
###### switch backup mode from incremental to full ######
incremental backup without validated full backup
INFO: copying database files
Expand All @@ -43,7 +51,7 @@ INFO: Please execute 'pg_rman validate' to verify the files are correctly copied
0
1
1
###### BACKUP COMMAND TEST-0008 ######
###### BACKUP COMMAND TEST-0009 ######
###### switch backup mode from archive to full ######
archive backup without validated full backup
ERROR: cannot take an incremental backup
Expand All @@ -61,7 +69,7 @@ INFO: Please execute 'pg_rman validate' to verify the files are correctly copied
0
1
1
###### BACKUP COMMAND TEST-0009 ######
###### BACKUP COMMAND TEST-0010 ######
###### failure in backup with different system identifier database ######
ERROR: could not start backup
DETAIL: system identifier of target database is different from the one of initially configured database
Expand Down
2 changes: 1 addition & 1 deletion pg_rman.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ extern void pgBackupValidate(pgBackup *backup, bool size_only, bool for_get_time
/* in catalog.c */
extern pgBackup *catalog_get_backup(time_t timestamp);
extern parray *catalog_get_backup_list(const pgBackupRange *range);
extern pgBackup *catalog_get_last_full_backup(parray *backup_list);
extern pgBackup *catalog_get_last_data_backup(parray *backup_list);
extern pgBackup *catalog_get_last_arclog_backup(parray *backup_list);
extern pgBackup *catalog_get_last_srvlog_backup(parray *backup_list);

Expand Down
23 changes: 18 additions & 5 deletions sql/backup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ pg_rman validate -B ${BACKUP_PATH} --quiet
pg_rman show detail -B ${BACKUP_PATH} > ${TEST_BASE}/TEST-0001.log 2>&1
grep -c OK ${TEST_BASE}/TEST-0001.log


echo '###### BACKUP COMMAND TEST-0002 ######'
echo '###### incremental backup mode ######'
pg_rman backup -B ${BACKUP_PATH} -b incremental -p ${TEST_PGPORT} -d postgres --quiet;echo $?
Expand All @@ -142,6 +141,20 @@ pg_rman show detail -B ${BACKUP_PATH} > ${TEST_BASE}/TEST-0004.log 2>&1
grep -c OK ${TEST_BASE}/TEST-0004.log

echo '###### BACKUP COMMAND TEST-0005 ######'
echo '###### Make sure incremental backup work ######'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the following better?

Make sure that pg_rman doesn't take a differential backup, but a incremental backup

init_catalog
pg_rman backup -B ${BACKUP_PATH} -b full -p ${TEST_PGPORT} -d postgres --quiet;echo $?
pg_rman validate -B ${BACKUP_PATH} --quiet
psql -p ${TEST_PGPORT} -d postgres -c 'create table test (c1 int);'
psql -p ${TEST_PGPORT} -d postgres -c 'insert into test values(generate_series(1,1000000));'
pg_rman backup -B ${BACKUP_PATH} -b incremental -p ${TEST_PGPORT} -d postgres --quiet;echo $?
pg_rman validate -B ${BACKUP_PATH} --quiet
pg_rman backup -B ${BACKUP_PATH} -b incremental -p ${TEST_PGPORT} -d postgres --quiet;echo $?
pg_rman validate -B ${BACKUP_PATH} --quiet
pg_rman show detail -B ${BACKUP_PATH} > ${TEST_BASE}/TEST-0012.log 2>&1
grep -c 16kB ${TEST_BASE}/TEST-0012.log
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to comment why you take the backups twice.
Why don't you the following comments?

If a differential backup was taken, the size of third backup is same as the second one and the size is bigger (XXMB).
16kB is the base backup size if nothing was changed in the database cluster.

But, I didn't check the second sentence is right or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to check 16KB is the size of third backup.

For example,

cat ${TEST_BASE}/TEST-0012.log | tail -n 1 | awk '{print $5, $6}'

Though I didn't check if this works on the machine, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for you commend.
I test that command, but it didn't get the expected results.
So I didn't change it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I misunderstood the order of pg_rman show's backup lists.
We need head -n 4.

Why don't you following?

> pg_rman show detail > ~/tmp/detail.log 2>&1
> cat ~/tmp/detail.log  | head -n 4 | tail -n 1 | awk '{print $5, $6, $13}'  
INCR 16kB OK

If you change the argument number of awk, you can check arbitrary columns.


echo '###### BACKUP COMMAND TEST-0006 ######'
echo '###### full backup with compression ######'
init_catalog
pg_rman backup -B ${BACKUP_PATH} -b full -s -Z -p ${TEST_PGPORT} -d postgres --quiet;echo $?
Expand All @@ -150,15 +163,15 @@ pg_rman show detail -B ${BACKUP_PATH} > ${TEST_BASE}/TEST-0005.log 2>&1
grep -c OK ${TEST_BASE}/TEST-0005.log
grep OK ${TEST_BASE}/TEST-0005.log | grep -c true

echo '###### BACKUP COMMAND TEST-0006 ######'
echo '###### BACKUP COMMAND TEST-0007 ######'
echo '###### full backup with smooth checkpoint ######'
init_catalog
pg_rman backup -B ${BACKUP_PATH} -b full -s -C -p ${TEST_PGPORT} -d postgres --quiet;echo $?
pg_rman validate -B ${BACKUP_PATH} --quiet
pg_rman show detail -B ${BACKUP_PATH} > ${TEST_BASE}/TEST-0006.log 2>&1
grep -c OK ${TEST_BASE}/TEST-0006.log

echo '###### BACKUP COMMAND TEST-0007 ######'
echo '###### BACKUP COMMAND TEST-0008 ######'
echo '###### switch backup mode from incremental to full ######'
init_catalog
echo 'incremental backup without validated full backup'
Expand All @@ -171,7 +184,7 @@ pg_rman show detail -B ${BACKUP_PATH} > ${TEST_BASE}/TEST-0010.log 2>&1
grep OK ${TEST_BASE}/TEST-0010.log | grep FULL | wc -l
grep ERROR ${TEST_BASE}/TEST-0010.log | grep INCR | wc -l

echo '###### BACKUP COMMAND TEST-0008 ######'
echo '###### BACKUP COMMAND TEST-0009 ######'
echo '###### switch backup mode from archive to full ######'
init_catalog
echo 'archive backup without validated full backup'
Expand All @@ -184,7 +197,7 @@ pg_rman show detail -B ${BACKUP_PATH} > ${TEST_BASE}/TEST-0011.log 2>&1
grep OK ${TEST_BASE}/TEST-0011.log | grep FULL | wc -l
grep ERROR ${TEST_BASE}/TEST-0011.log | grep ARCH | wc -l

echo '###### BACKUP COMMAND TEST-0009 ######'
echo '###### BACKUP COMMAND TEST-0010 ######'
echo '###### failure in backup with different system identifier database ######'
init_catalog
pg_ctl stop -m immediate > /dev/null 2>&1
Expand Down