Skip to content

Allow Any Postgres Params #295

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

Merged
merged 9 commits into from
Nov 24, 2021
Merged

Allow Any Postgres Params #295

merged 9 commits into from
Nov 24, 2021

Conversation

eberlep
Copy link
Collaborator

@eberlep eberlep commented Nov 22, 2021

No description provided.

@eberlep eberlep linked an issue Nov 22, 2021 that may be closed by this pull request
@eberlep
Copy link
Collaborator Author

eberlep commented Nov 23, 2021

Overwriting of default params works as well:

Test (Control Plane):

 spec:
   postgresParams:
     pgaudit.log: all, -misc
     pgaudit.log_parameter: "off"

Result (Service Cluster):

 spec:
   postgresql:
     parameters:
       pgaudit.log: all, -misc
       pgaudit.log_catalog: "off"
       pgaudit.log_parameter: "off"
       pgaudit.log_relation: "on"
       shared_buffers: 64MB
       shared_preload_libraries: pgaudit

Default (Service Cluster):

 spec:
   postgresql:
     parameters:
       pgaudit.log: ddl
       pgaudit.log_catalog: "off"
       pgaudit.log_parameter: "on"
       pgaudit.log_relation: "on"
       shared_buffers: 64MB
       shared_preload_libraries: pgaudit

@eberlep
Copy link
Collaborator Author

eberlep commented Nov 23, 2021

Setting numeric values as strings seems to work as well:

Test (Control Plane):

 spec:
   postgresParams:
     max_connections: "1001"

Result (Service Cluster):

 spec:
   postgresql:
     parameters:
       max_connections: "1001"

Result (Patroni):

root@pgsampletenant-complete-0:/home/postgres# patronictl show-config
[..]
postgresql:
  parameters:
    [..]
    max_connections: '1001'
    [..]

Result (Database Instance):

postgres=# SHOW max_connections;
 max_connections 
-----------------
 1001
(1 row)

@eberlep eberlep requested a review from majst01 November 23, 2021 11:24

// setPostgresParams add the provided params to the parameter map
func setPostgresParams(parameters map[string]string, providedParams map[string]string) {
for k, v := range providedParams {
Copy link
Contributor

Choose a reason for hiding this comment

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

My feeling is that there should be at least be a denylist of params be configurable which a user cannot override

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the central cloud-api or locally in each postgreslet/service cluster? Locally is more flexible, but also more difficult to maintain.

Copy link
Contributor

Choose a reason for hiding this comment

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

unsure about this

Copy link
Contributor

Choose a reason for hiding this comment

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

implementation wise, locally in a configmap is easier, deployment is done via playbook, so keeping this configmap identical between installation should also be a easy task

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used viper to read a (apparently space-separated) list of blocked values from the environment, convert it into a map for easier use and pass that map along to the code in question. now before setting a given postgres param, we check if is contained in the blocklist. if it is, we simply ignore it.

Copy link
Contributor

@majst01 majst01 left a comment

Choose a reason for hiding this comment

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

parameters which are not allowed to change should be considered, at least in a slice in the configmap

@eberlep eberlep requested a review from majst01 November 24, 2021 13:45
@eberlep eberlep merged commit 2351140 into main Nov 24, 2021
@eberlep eberlep linked an issue May 12, 2022 that may be closed by this pull request
@eberlep eberlep deleted the 276-allow-any-postgres-params branch August 19, 2022 12:21
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.

Allow Any Postgres Params Make KeepAlive Configurable
2 participants