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

Correct log message for PG restart on replication config changes #1934

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NikolayS
Copy link
Contributor

When some replication-related changes happen (e.g., restore_command
is changed), Patroni restarts Postgres and the following can bee seen in logs:

INFO: changing primary_conninfo and restarting in progress

This message is misleading because it mentions one particular parameter
but the change could happen with a different one (e.g., restore_command).

This is a quick improvement that makes the messaging better. In the future,
it would make sense to report the diff of changes (hence, particular root cause
of the restart) that is analyzed in config.py:record_missmatch.

When some replication-related changes happen (e.g., restore_command
is changed), Patroni restarts Postgres and the following can bee seen in logs:

INFO: changing primary_conninfo and restarting in progress   

This message is misleading because it mentions one particular parameter
but the change could happen with a different one (e.g., restore_command).

This is a quick improvement that makes the messaging better. In the future,
it would make sense to report the diff of changes (hence, particular root cause
of the restart) that is analyzed in config.py:record_missmatch.
@NikolayS
Copy link
Contributor Author

@CyberDem0n we've discussed this in person – what do you think to adjust the log message (maybe making it a bit shorter), while planning to implement config diff reported in logs explicitly? (The main idea I have is that we need to see what exactly has changed – but I guess it might be non-trivial and long, so let's have some simple improvement first).

@NikolayS
Copy link
Contributor Author

NikolayS commented May 10, 2021

A shorter message could be just: changing replication-related parameters and restarting

The list of params being checked can be found here:

def record_missmatch(mtype):

What I see there:

  • archive_cleanup_command
  • promote_trigger_file
  • recovery_end_command
  • restore_command
  • recovery_min_apply_delay
  • standby_mode
  • primary_slot_name

Copy link
Collaborator

@CyberDem0n CyberDem0n left a comment

Choose a reason for hiding this comment

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

@NikolayS if we go this way, I believe it would be also nice to log what parameters have been changed in the check_recovery_conf() method.
Only parameter names, without values (values could be sensitive!), and only if change results in a restart.

Comment on lines +429 to +430
self._async_executor.try_run_async('replication-related parameters (such as primary_conninfo, restore_command) has recently changed, '
'restarting PostgreSQL to apply the changes',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self._async_executor.try_run_async('replication-related parameters (such as primary_conninfo, restore_command) has recently changed, '
'restarting PostgreSQL to apply the changes',
self._async_executor.try_run_async('changing replication-related parameters and restarting',

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.

None yet

2 participants