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

Update supautils.conf.j2 | added log_replication_commands to supautils #1408

Merged
merged 2 commits into from
Mar 13, 2025

Conversation

TheOtherBrian1
Copy link
Contributor

What kind of change does this PR introduce?

supautils update

What is the current behavior?

Cannot configure log_replication_commands

What is the new behavior?

Added log_replication_commands to the list

Additional context

This should work with replicas because it's set at the role level. Context:

@TheOtherBrian1 TheOtherBrian1 requested a review from a team as a code owner January 14, 2025 01:16
@samrose
Copy link
Collaborator

samrose commented Mar 5, 2025

@TheOtherBrian1 I wonder what we can do to resolve this one? Still waiting for review?

@TheOtherBrian1
Copy link
Contributor Author

TheOtherBrian1 commented Mar 5, 2025

@samrose, it's been a few weeks since I looked at this. Yeah, I'm still waiting for a review. The original ticket that prompted the PR was from an enterprise customer that was having trouble debugging their replication. With Postgres 17 coming out soon, this option can better enable users to self-manage

@samrose
Copy link
Collaborator

samrose commented Mar 8, 2025

@doublethink @staaldraad @darora @pcnc @olirice @soedirgo

Can you take a look at this and see if you have any concerns about compliance or security leakage around enabling log_replication_commands for privileged roles please?

@staaldraad
Copy link
Contributor

@doublethink @staaldraad @darora @pcnc @olirice @soedirgo

Can you take a look at this and see if you have any concerns about compliance or security leakage around enabling log_replication_commands for privileged roles please?

I think we are good with enabling this without too much risk.

@soedirgo
Copy link
Member

Also OK with enabling this

Copy link
Contributor

@darora darora left a comment

Choose a reason for hiding this comment

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

Is it desirable to be setting all of these at the role level?

No particular objection to allowing it, but we keep permitting config here that in a lot of cases makes more sense to set at the DB level (e.g. auto_explain, log_lock_waits, log_min_duration)

@soedirgo
Copy link
Member

Is it desirable to be setting all of these at the role level?

No strong preference for either approach, but setting it at the role level is marginally safer (e.g. no undesired config changes on internal roles)

@soedirgo soedirgo merged commit dc011e6 into develop Mar 13, 2025
12 checks passed
@soedirgo soedirgo deleted the TheOtherBrian1-patch-1 branch March 13, 2025 10:33
@darora
Copy link
Contributor

darora commented Mar 14, 2025

No strong preference for either approach, but setting it at the role level is marginally safer (e.g. no undesired config changes on internal roles)

Safer, yes, but less useful (e.g. would you not want log_lock_waits set for pgrst usage?). In any case, doesn't block this particular change.

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.

5 participants