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.

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

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Conversation

zwyan0
Copy link
Contributor

@zwyan0 zwyan0 commented Jun 9, 2022

To support PostgreSQL15

Copy link
Contributor

@mikecaat mikecaat left a 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!

.github/workflows/build.yml Outdated Show resolved Hide resolved
@mikecaat mikecaat changed the title For support PostgreSQL15 To support PostgreSQL v15 Jun 13, 2022
Copy link
Contributor

@mikecaat mikecaat left a 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.

sql/show.sh Outdated Show resolved Hide resolved
backup.c Outdated Show resolved Hide resolved
backup.c Outdated Show resolved Hide resolved
backup.c Outdated Show resolved Hide resolved
backup.c Outdated Show resolved Hide resolved
@mikecaat
Copy link
Contributor

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.
@zwyan0
Copy link
Contributor Author

zwyan0 commented Jul 21, 2022

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.

I changed commit message.

Copy link
Contributor

@mikecaat mikecaat left a 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
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 we don't need this comment because the function is renamed and we can't execute it to other versions.

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 deleted this comment.

backup.c Outdated
/*
* Assumes PG version >= 8.4
*
* When second parameter is given as true,
Copy link
Contributor

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

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

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?

Copy link
Contributor

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.

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 checked all files. And I think only this place need to change the version to 15.0.

backup.c Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
backup.c Show resolved Hide resolved
backup.c Show resolved Hide resolved
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Delete the version 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.

I deleted this comment.

backup.c Show resolved Hide resolved
zwyan0 and others added 11 commits July 28, 2022 13:49
* 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.
@tdchi
Copy link

tdchi commented Dec 5, 2022

Hello.
When will the pg_rman source be released to support PG15?

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.
@zwyan0
Copy link
Contributor Author

zwyan0 commented Dec 9, 2022

Hello. When will the pg_rman source be released to support PG15?

Hello.
I can't give you an exact time, but we will try to release it as soon as possible.

.github/workflows/build.yml Outdated Show resolved Hide resolved
backup.c Show resolved Hide resolved
・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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants