Get the correct increment backup #176

huangfumingyue · 2021-05-18T01:20:27Z

Before, we actually got the difference backup, not the incremental backup.
TO solve this probem ,revert #125.

However, if a physical backup file is deleted(rm command, etc.) other than the pg_rman delete command of pg_rman,
It cannot find the correct status of a full backup and incremental backup.(please refer to #154 to details)
WE will solve this problem, at the next major upgarde.

Before, we actually got the difference backup, not the incremental backup. TO solve this probem ,revert #125. However, if a physical backup file is deleted(rm command, etc.) other than the pg_rman delete command of pg_rman, It cannot find the correct status of a full backup and incremental backup.(please refer to #154 to details) WE will solve this problem, at the next major upgarde.

get the correct increment backup

…backup Revert "get the correct increment backup"

Before, we actually got the difference backup, not the incremental backup. TO solve this probem ,revert #125. However, if a physical backup file is deleted(rm command, etc.) other than the pg_rman delete command of pg_rman, It cannot find the correct status of a full backup and incremental backup.(please refer to #154 to details) WE will solve this problem, at the next major upgarde.

MoonInsung

Dear @huangfumingyue .
Thank you for responding to this issue.

Your opinion of reverting the previous commit looks good.
The problem of acquiring meaningless incremental backups is occurred due to revert, but it seems that solving this problem is not simple.
In this regard, I have previously registered an issue #154, and we need to discuss it later to resolve the issue.

MoonInsung · 2021-05-20T00:17:44Z

catalog.c

@@ -266,7 +266,7 @@ catalog_get_backup_list(const pgBackupRange *range)
 * Find the last completed database full valid backup from the backup list.
 */


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.

MoonInsung · 2021-05-20T00:20:23Z

And, please add a regression test.
Currently, there are no tests to verify that incremental backups have been acquired normally.
Therefore, please add a test to check the size of the incremental backup.

add the comment about the increment backup's problem

mikecaat

Thanks for making your patches

mikecaat · 2021-05-24T08:23:01Z

sql/backup.sh

+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


I think it's better to check more precisely. Why don't you check each size of data, arclog, and srvlog?

mikecaat · 2021-05-24T08:27:45Z

sql/backup.sh

+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


Because the size of the data can be bigger or smaller in the future, is it better to check with a certain range of data size? For example, check if more than 20kB and less than 30kB.

In many environments, I confirmed that the acquired data size is 16kB when there is no update.
And I cannot find the command to check the number is more than 20kB and less than 30kB.

It's OK since this seems to be related to pg_rman's code.

mikecaat · 2021-05-24T08:28:25Z

backup.c

@@ -185,14 +185,18 @@ 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 a problem of increment backup.
+	 * IF a physical backup file is deleted(rm command, etc.) other than the pg_rman delete command of pg_rman, 


IF is capital. Is it intentional?

It'll happen even if using pg_rman delete command. The delete command has the dangerous '--force' option.

Thank you for you comment. I already fix it..

mikecaat · 2021-05-24T08:29:01Z

backup.c

@@ -185,14 +185,18 @@ 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 a problem of increment backup.


There "is" a ? I think it's better to refer to the issue number.

Thank you for you comment. I already fix it..

mikecaat · 2021-05-24T08:39:34Z

sql/backup.sh

@@ -184,7 +184,20 @@ 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-0009 #######'
+echo '###### confirm incremental backup is right #######'


Is it better to change from "is right" to "works"?

Thank you for you comment. I already fix it..

mikecaat · 2021-05-24T08:45:56Z

sql/backup.sh

+init_catalog
+pg_rman backup -B ${BACKUP_PATH} -b full -p ${TEST_PGPORT} -d postgres --quiet;echo $?
+pg_rman validate -B ${BACKUP_PATH} --quiet
+pgbench -i -s 50 -d postgres >  ${TEST_BASE}/pgbench.log 2>&1


Why do you set the scale factor as 50? If you can, I think It's better to use a smaller value because the test execution time can be shortened.

Thank you for your comment.
I changed the SQL.

Add a comment

add the regression test of increment backup

becaust upload the wrong place,delete it

mikecaat

Thanks for your work! I have some comments.

mikecaat · 2021-05-25T08:57:20Z

backup.c

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


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?

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

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.

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

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.

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

mikecaat · 2021-05-25T09:08:57Z

sql/backup.sh

@@ -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 ######'


Is the following better?

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

mikecaat · 2021-05-25T09:12:08Z

sql/backup.sh

+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


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.

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.

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

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.

MoonInsung · 2021-05-28T00:33:19Z

Dear @huangfumingyue , @mikecaat

Thank you for the response and review!!

I think Issues related to incremental backups remain(#154) but like this PR,
(1) Rollback of commits related to incremental backup
(2) Add comments on the problems related to incremental backup
(3) Add regression test related to incremental backup
We need to deal with the most urgent problems like #150.

So, I look good about the modified PR.

If, don't have any opinion, I'll commit it aroud 15:00(JST) on May 28.
Also, I'll do a back-patch REL_9_6_STABLE to REL_13_STABLE for these PR commits.

Best regards.
Moon.

* get the correct increment backup Before, we actually got the difference backup, not the incremental backup. TO solve this probem ,revert #125. However, if a physical backup file is deleted(rm command, etc.) other than the pg_rman delete command of pg_rman, It cannot find the correct status of a full backup and incremental backup.(please refer to #154 to details) WE will solve this problem, at the next major upgarde. * Revert "get the correct increment backup" * get the correct increment backup Before, we actually got the difference backup, not the incremental backup. TO solve this probem ,revert #125. However, if a physical backup file is deleted(rm command, etc.) other than the pg_rman delete command of pg_rman, It cannot find the correct status of a full backup and incremental backup.(please refer to #154 to details) WE will solve this problem, at the next major upgarde. * add the regression test of increment backup * add the regression test of increment backup * add the comment add the comment about the increment backup's problem * Add a comment Add a comment * add the regression test of increment backup * add the regression test of increment backup add the regression test of increment backup * add the comment * add the regression test * delete file becaust upload the wrong place,delete it * add the regression test * Reflected the comment * Reflected the comment * Reflected the comment * Reflected the comment * Reflected the comment * Reflected the comment * Reflected the comment * Regression test command was modified * Regression test command was modified * Regression test command was modified

huangfumingyue added 5 commits May 18, 2021 09:35

Merge pull request #3 from huangfumingyue/fix-the-increment-backup

2b2078d

get the correct increment backup

Revert "get the correct increment backup"

6d74b7f

Merge pull request #4 from huangfumingyue/revert-3-fix-the-increment-…

48008dc

…backup Revert "get the correct increment backup"

MoonInsung reviewed May 20, 2021

View reviewed changes

huangfumingyue added 3 commits May 24, 2021 15:54

add the regression test of increment backup

4aa9439

add the regression test of increment backup

84619bc

add the comment

24fd3b4

add the comment about the increment backup's problem

mikecaat reviewed May 24, 2021

View reviewed changes

huangfumingyue added 7 commits May 25, 2021 14:11

Add a comment

3372846

Add a comment

add the regression test of increment backup

e911da8

add the regression test of increment backup

b5e24ba

add the regression test of increment backup

add the comment

dabee15

add the regression test

5099504

delete file

b813cb9

becaust upload the wrong place,delete it

add the regression test

6a4007b

mikecaat reviewed May 25, 2021

View reviewed changes

huangfumingyue added 7 commits May 26, 2021 10:54

Reflected the comment

7aa3475

Reflected the comment

1435b92

Reflected the comment

e197388

Reflected the comment

cf92d36

Reflected the comment

aa2e6c4

Reflected the comment

482dbb6

Reflected the comment

5b95534

MoonInsung mentioned this pull request May 27, 2021

fix issue 149 and 150 #151

Closed

Regression test command was modified

17ac111

huangfumingyue added 2 commits May 27, 2021 13:14

Regression test command was modified

a23a8ea

Regression test command was modified

622c787

MoonInsung merged commit ec0c350 into ossc-db:master May 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get the correct increment backup #176

Get the correct increment backup #176

huangfumingyue commented May 18, 2021

MoonInsung left a comment •

edited

Loading

MoonInsung May 20, 2021

MoonInsung commented May 20, 2021

mikecaat left a comment

mikecaat May 24, 2021

mikecaat May 24, 2021

huangfumingyue May 25, 2021

mikecaat May 25, 2021

mikecaat May 24, 2021

mikecaat May 24, 2021

huangfumingyue May 25, 2021

mikecaat May 27, 2021

mikecaat May 24, 2021

huangfumingyue May 25, 2021

mikecaat May 25, 2021

mikecaat May 24, 2021 •

edited

Loading

huangfumingyue May 25, 2021

mikecaat May 25, 2021

mikecaat May 24, 2021

huangfumingyue May 25, 2021

mikecaat May 27, 2021

mikecaat left a comment

mikecaat May 25, 2021 •

edited

Loading

huangfumingyue May 26, 2021

MoonInsung May 26, 2021

mikecaat May 27, 2021 •

edited

Loading

MoonInsung May 27, 2021

mikecaat May 27, 2021

mikecaat May 25, 2021

mikecaat May 25, 2021

mikecaat May 25, 2021

huangfumingyue May 26, 2021

mikecaat May 27, 2021

MoonInsung commented May 28, 2021

		@@ -266,7 +266,7 @@ catalog_get_backup_list(const pgBackupRange *range)
		* Find the last completed database full valid backup from the backup list.
		*/

Get the correct increment backup #176

Get the correct increment backup #176

Conversation

huangfumingyue commented May 18, 2021

MoonInsung left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonInsung commented May 20, 2021

mikecaat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikecaat May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikecaat left a comment

Choose a reason for hiding this comment

mikecaat May 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikecaat May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonInsung commented May 28, 2021

MoonInsung left a comment •

edited

Loading

mikecaat May 24, 2021 •

edited

Loading

mikecaat May 25, 2021 •

edited

Loading

mikecaat May 27, 2021 •

edited

Loading