-
Notifications
You must be signed in to change notification settings - Fork 77
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.
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
To support PostgreSQL v15 #229
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your great work to support v15!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your works!
Although I have some comments, it's ok after the RC version is released because it's trivial things.
Please add commit message the deatailed reason why you change the code because mainteners may change. For example, I think the following things are important.
-
why do we change to pg_backup_start()/pg_backup_stop() and the arguments are changed?
In my understanding, though the exclusive mode is remove, it just renames the function name and keeps the behavior because pg_rman already changed to use non-exclusive mode. -
why the arc_srv_log_management.sh change the
log_checkpoints = off
? In my undersanding, the reason is that the test to check if the expected server logs are removed fails since the default value bacame 'on'. The checkpoint logs update the timestamp of server log files when executing the test, and it makes remove newer dummy server logs. To avoid the behavior, turn it off only when the tests are executed.
Thanks! |
・Renamed the functions pg_start_backup()/pg_stop_backup() to pg_backup_start()/pg_backup_stop() and change the arguments of pg_backup_stop() pg_rman already supported non-exclusive mode, so we just renamed the function names and change the arguments to keeps the behavior. ・Default log_checkpoints for regression test is off. In PostgreSQL15, the default value of log_checkpoints is on. The checkpoints are automatically triggered when regression tests are run. The test failed because some log files was updated timestamp and the wrong log was deleted. So, we turned this feature off before running the test.
I changed commit message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I forgot to public my comments.
backup.c
Outdated
|
||
/* 2nd argument is 'fast' (IOW, !smooth) */ | ||
/* | ||
* Assumes PG version >= 8.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this comment because the function is renamed and we can't execute it to other versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted this comment.
backup.c
Outdated
/* | ||
* Assumes PG version >= 8.4 | ||
* | ||
* When second parameter is given as true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this comment? I think the old comment is enough because we can see the description in the official postgresql documents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted this comment.
backup.c
Outdated
* either the primary or standby server to successfully archive the last | ||
* needed WAL segment to be archived. Returns once that's been done. | ||
* | ||
* As of PG version 9.6, pg_stop_backup() returns 2 more fields in addition | ||
* As of PG version 9.6, pg_backup_stop() returns 2 more fields in addition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do need to change the version from 9.6 to 15.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check if there are other messages related to other PG version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked all files. And I think only this place need to change the version to 15.0.
backup.c
Outdated
* either the primary or standby server to successfully archive the last | ||
* needed WAL segment to be archived. Returns once that's been done. | ||
* | ||
* As of PG version 9.6, pg_stop_backup() returns 2 more fields in addition | ||
* As of PG version from 15.0, pg_backup_stop() returns 2 more fields in addition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete the version number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted this comment.
* Add notes to take a backup at standby-site * Unify 'standby site' to 'standby-site' Co-authored-by: Masahiro Ikeda <masahiro.ikeda.us@hco.ntt.co.jp>
Previously, if the server has a tablespace and its path has PGDATA's path, the restore fails because mkdirs.sh doesn't work. For example, the following case fails to restore. * PGDATA path: /tmp/pgdata * tablespace path: /tmp/pgdata_tblspc So, this patch fixes the case. The root cause is the logic checking whether the file taking backup is in tablespace or PGDATA. Because it only considers their prefix, if the prefix path of tablespace is the same as PGDATA, it decides the file is in PGDATA. So, this patch changes the logic to check it has '/' after the prefix.
・Renamed the functions pg_start_backup()/pg_stop_backup() to pg_backup_start()/pg_backup_stop() and change the arguments of pg_backup_stop() pg_rman already supported non-exclusive mode, so we just renamed the function names and change the arguments to keeps the behavior. ・Default log_checkpoints for regression test is off. In PostgreSQL15, the default value of log_checkpoints is on. The checkpoints are automatically triggered when regression tests are run. The test failed because some log files was updated timestamp and the wrong log was deleted. So, we turned this feature off before running the test.
Hello. |
The following commit(17f7109) forgot to change the index of parameters although it adds a new parameter. It makes --hard-copy not work in restore. And there is no test code to check that the default option uses symbolic links. So add some test code and modified the manual about describe the -Z option.
Hello. |
・Renamed the functions pg_start_backup()/pg_stop_backup() to pg_backup_start()/pg_backup_stop() and change the arguments of pg_backup_stop() pg_rman already supported non-exclusive mode, so we just renamed the function names and change the arguments to keeps the behavior. ・Default log_checkpoints for regression test is off. In PostgreSQL15, the default value of log_checkpoints is on. The checkpoints are automatically triggered when regression tests are run. The test failed because some log files was updated timestamp and the wrong log was deleted. So, we turned this feature off before running the test.
…nto test_pg15_bate1
To support PostgreSQL15