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 8 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
8 changes: 6 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,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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 comment. I already fix it..

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

* IF a physical backup file is deleted(rm command, etc.) other than the pg_rman delete command of pg_rman,
Copy link
Contributor

Choose a reason for hiding this comment

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

IF is capital. Is it intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 comment. I already fix it..

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.

* It cannot find the correct status of a full backup and incremental backup.
* we need to discuss it later to resolve this problem.
*/
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
8 changes: 7 additions & 1 deletion expected/backup.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,13 @@ INFO: Please execute 'pg_rman validate' to verify the files are correctly copied
0
1
1
###### BACKUP COMMAND TEST-0009 ######
###### BACKUP COMMAND TEST-0009 #######
###### confirm incremental backup is right #######
0
0
0
1
###### 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
15 changes: 14 additions & 1 deletion sql/backup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 #######'
Copy link
Contributor

@mikecaat mikecaat May 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

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 comment. I already fix it..

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 your comment.
I changed the SQL.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.

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 check more precisely. Why don't you check each size of data, arclog, and srvlog?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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 OK since this seems to be related to pg_rman's code.


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