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

The default of SRVLOG_PATH should be log? #174

Open
atorik opened this issue May 13, 2021 · 6 comments
Open

The default of SRVLOG_PATH should be log? #174

atorik opened this issue May 13, 2021 · 6 comments

Comments

@atorik
Copy link
Contributor

atorik commented May 13, 2021

When I set up pg_rman, SRVLOG_PATH was set to pg_log.

ISTM pg_rman assumes the default of SRVLOG_PATH is pg_log, but it has changed to log since PostgreSQL 10.

--init.c
177                 else if (pgdata)
178                 {
179                         /* default: log_directory = 'pg_log' */
180                         srvlog_path = pgut_malloc(MAXPGPATH);
181                         join_path_components(srvlog_path, pgdata, "pg_log");
182                 }

Is there any intention to keep it to pg_log?

@atorik atorik changed the title SRVLOG_PATH The default of SRVLOG_PATH should be log? May 13, 2021
@mikecaat
Copy link
Contributor

mikecaat commented May 14, 2021

Hi,

Thanks for reporting! I agree it should be log.
It seems to lead following issues if the "log_directory" is not specified.

  1. Server logs are always backuped even if "--with-serverlog" is not specified because pg_rman fails to exclude the "PGDATA/log" directory from backup target lists.
  2. pg_rman's "--keep-srvlog-files" and "--keep-srvlog-days" options are invalid because SRVLOG_PATH is wrong.

Well, related to this issue, I found pg_rman doesn't restore the server log files backed up (although it remains online log files). I think this should be fixed too. Now, since SRVLOG_PATH is wrong, pg_rman handles the server logs as data files and it restores the server logs. But if to change SRVLOG_PATH from pg_log to log, it don't restore the server logs because it seems not to have the logic restoring them...

Although there is a possibility this behavior is intentional or I'm missing something, I think to support restoring the server logs is natural for me.

If my understanding is right, there are two choices.

a. Only to change SRVLOG_PATH from pg_log to log. The side effect is that users have to check the server logs backed up in the $BACKUP_PATH directory because they are not restored. And it remains online server log files, is it enough?

b. To change SRVLOG_PATH from pg_log to log and add the logic to restoring the server log files. There is a possibility that some users don't want this feature.

Though?

@atorik
Copy link
Contributor Author

atorik commented May 18, 2021

Thanks for investigating it!

a. Only to change SRVLOG_PATH from pg_log to log. The side effect is that users have to check the server logs backed up in the $BACKUP_PATH directory because they are not restored. And it remains online server log files, is it enough?

b. To change SRVLOG_PATH from pg_log to log and add the logic to restoring the server log files. There is a possibility that some users don't want this feature.

IMHO, I like b.

As you worry, I also imagine some people don't want to restore log files because restoring log files consume time and it may influence RTO and they might be backed up not by pg_rman but other log collecting system.

Considering this, it might be better if users could choose to include log files when restoring.

@MoonInsung
Copy link
Contributor

Hello.

Thank you for reporting issues and suggesting solutions.

I think this problem occurred because I did not modify
the default path for archived server log when PG10 or higher versions.
Also, the server log is not restored normally due to
this related issue being likely to be a very big issue depending on the user.

We plan to release a new minor version in May.
Therefore, since I think that discussion and correction about the restore need some time,
And first, I want to modify the server log's default path for the branch of PG10 or higher versions.
After that, we'd like to resolve issues related to the restore.

What do you think of this opinion?

Best regards.
Moon.

@mikecaat
Copy link
Contributor

mikecaat commented May 21, 2021

Hi,

Thanks for your comments!
OK, I agreed.

Is it better to make a new pull request?
I think you can cherry-pick 2b1d186, though.

Regards,

@MoonInsung
Copy link
Contributor

Dear @mikecaat .

Thank you for agreeing to my opinion.

Yes. I am going to use cherry-pick(2b1d186).
Therefore, we can proceed with revisions and discussions
related to the store using the previously generated PR(#175).

If there is no comment,
I will commit only the patch related to the log default path around 11:00 JST on May 24.

Best regards.
Moon.

@MoonInsung
Copy link
Contributor

I just committed 2b1d186 only.
This commit will be back-patches to
REL_10_STABLE, REL_11_STABLE, REL_12_STABLE, and REL_13_STABLE branches.

And, I want to make a solution after a little more discussion for issues related to log restore.

Best regards.
Moon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants