Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

make postgresql configurable #59

Closed
wants to merge 1 commit into from
Closed

make postgresql configurable #59

wants to merge 1 commit into from

Conversation

scoopex
Copy link
Contributor

@scoopex scoopex commented Jun 23, 2022

Solves #58

What this PR does / why we need it:

  • provide the possibility to use a alternate configuration file for postgresql
  • provide the possibility to integrate extra volumes for postgresql (useful for configuration files, and migration scenarios with pg_upgrade)

Which issue this PR fixes

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md

@scoopex
Copy link
Contributor Author

scoopex commented Jun 24, 2022

@sa-ChristianAnton
Copy link
Contributor

I actually like the idea to make the "included" postgresql more configurable, but I have some remarks on the implementation you did:

  1. The possibility to add additional volumes, volumeMounts (and also side car containers) to all of the components deployed with this helm chart has been implemented already in PR #54, which we are almost about to merge into master.
  2. In order to keep up with the overall concepts, instead of having a fixed -c config_file.... argument given to all deployments, I would prefer having one list of postgresql.extraArguments (default empty) inside the values.yaml which you can use to add whatever arguments to the postgresql command. Config files is something to avoid if possible in cloud native deployments, but would still be entirely possible to do with this implementation. For those that want additional settings to be supplied to the postgresql service without a config file, that would also work.

@aeciopires What do you think? How could we make that happen?

@scoopex
Copy link
Contributor Author

scoopex commented Jun 26, 2022

It's great to see that the helm chart is getting better.
If this PR is obsolete with #54 thats o.k.

From my point of view it would be a good thing to give users of the helm chart the hint in the documentation that it is recommended to configure the parameters of postgresql to prevent bad performance.

I configure the following things to run 200 items/sec setup (8GB requests/limit resources):

listen_addresses = '*'
dynamic_shared_memory_type = posix
max_connections = 300
shared_buffers = 4GB
temp_buffers = 16MB
# provides the amount of memory to be used by internal sort operations and hash tables
# before writing to temporary disk files.
# Sort operations are used for order by, distinct, and merge join operations.
# Hash tables are used in hash joins and hash based aggregation
work_mem = 128MB # 4MB
maintenance_work_mem = 256MB
# estimates how much memory is available for disk caching
# by the operating system and within the database itself.
effective_cache_size = 6GB
min_wal_size = 80MB
log_timezone = 'Europe/Berlin'
log_line_prefix = '"%t [%p]: [%l-1] user=%u,db=%d,app=%a,client=%h"'
datestyle = 'iso, mdy'

@sa-ChristianAnton
Copy link
Contributor

I have added commit d90e63d to the other pull request I was working on, as it affects/extends one setting that I had introduced within the same: ability of setting the max_connections.
@aeciopires I hope that's OK with you... kind of a last-minute change though.

@aeciopires
Copy link
Collaborator

Hey guys!

Thanks @scoopex for report issue and open this Pull Request with solution.

I agree with the suggestion made by @sa-ChristianAnton in PR #54. This allows tuning of postgresql configuration desired by @scoopex and allows it to be applied to other Zabbix components that use postgresql and still leaves the helm chart code robust, but at least centralized and with low code repetition and configuration.

Could @scoopex a good look at the PR and evaluate/test if what we are changing in version 3.0.0 meets your needs?

If something is missing, could you give your opinion on what we can do or contribute with code in this PR.

From what I've seen the changes made today, we're pretty close to doing the merge. I will be carrying out the tests this afternoon (Brazil time) and if everyone is in agreement and there are no changes to be made after the tests, I believe we can publish the new version this week.

@aeciopires
Copy link
Collaborator

This PR will not merge. The problem was solved in PR #54

@aeciopires aeciopires closed this Jun 26, 2022
@scoopex
Copy link
Contributor Author

scoopex commented Jun 26, 2022

#54 looks good to me because i can define extra volumes/volumeMounts and i can configure a extra cofiguration file.

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

Successfully merging this pull request may close these issues.

3 participants