make postgresql configurable

scoopex commented 2 years ago

Solves https://github.com/cetic/helm-zabbix/issues/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

58

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 commented 2 years ago

@aeciopires @AyadiAmen @AyadiAmen @banzo

sa-ChristianAnton commented 2 years ago

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

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.
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 commented 2 years ago

It's great to see that the helm chart is getting better. If this PR is obsolete with https://github.com/cetic/helm-zabbix/pull/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

scoopex commented 2 years ago

https://github.com/cetic/helm-zabbix/pull/54 looks good to me because i can define extra volumes/volumeMounts and i can configure a extra cofiguration file.

cetic / helm-zabbix