ANXS / postgresql

Fairly full featured Ansible role for Postgresql.
http://anxs.io/
MIT License
848 stars 571 forks source link

support PostgreSQL 16 #565

Closed conscribtor closed 5 months ago

conscribtor commented 6 months ago

Hello there!

I'm working in my fork on a PR to contribute a configuration update that would support PostgreSQL 16: #564 Please let me know if I'm going about it in the wrong way.

I've used the opportunity to (re)check the PostgreSQL documentation and templates for all versions greater or equal 10 and found some defaults that don't seem correct to me. However, changes to these might affect existing installations and therefore I'd be happy on some feedback before I commit and test these changes.

# does not seem to be used:
- postgresql_service_enabled: true

# default changed in version 14
- postgresql_krb_server_keyfile: ""
+ postgresql_krb_server_keyfile: "{{ 'FILE:${sysconfdir}/krb5.keytab' if postgresql_version is version_compare('14', '>=') else '' }}"

# default changed in version <= 10
- postgresql_work_mem:                   1MB      # min 64kB
+ postgresql_work_mem:                   4MB      # min 64kB

# default changed in version 15
- postgresql_hash_mem_multiplier:        1.0      # (>= 13)
+ postgresql_hash_mem_multiplier:        "{{ 2.0 if postgresql_version is version_compare('15', '>=') else 1.0 }}" # (>= 13)

# default changed in version <= 10
- postgresql_maintenance_work_mem:       16MB     # min 1MB
+ postgresql_maintenance_work_mem:       64MB     # min 1MB

# default changed in version 14
- postgresql_vacuum_cost_page_miss:  10      # 0-10000 credits
+ postgresql_vacuum_cost_page_miss:  "{{ 2 if postgresql_version is version_compare('14', '>=') else 10 }}" # 0-10000 credits

# default changed in version 14
- postgresql_checkpoint_completion_target: 0.5   # checkpoint target duration, 0.0 - 1.0
+ postgresql_checkpoint_completion_target: "{{ 0.9 if postgresql_version is version_compare('14', '>=') else 0.5 }}" # checkpoint target duration, 0.0 - 1.0

# default changed in version <= 10
- postgresql_hot_standby: off
+ postgresql_hot_standby: on

# default changed in version <= 10
- postgresql_effective_cache_size:       128MB
+ postgresql_effective_cache_size:         4GB

# default changed in version <= 10
- postgresql_log_directory:              "pg_log"
+ postgresql_log_directory:              "log"

# default changed in version 15
- postgresql_log_autovacuum_min_duration: -1
+ postgresql_log_autovacuum_min_duration: "{{ '10min' if postgresql_version is version_compare('15', '>=') else -1 }}"

# default changed in version 15
- postgresql_log_checkpoints:       off
+ postgresql_log_checkpoints:       "{{ 'on' if postgresql_version is version_compare('15', '>=') else 'off' }}"
# default changed in version <= 10
- postgresql_log_line_prefix: "%t "
+ postgresql_log_line_prefix: "%m [%p] "

# default changed in version 12
- postgresql_extra_float_digits: 0
+ postgresql_extra_float_digits: "{{ 1 if postgresql_version is version_compare('12', '>=') else 0 }}"

Should I change the defaults to be valid for versions >= 16, ensuring backwards compatibility, or should I "correct" (some are surely a matter of opinion) them to their historic defaults?

MrMegaNova commented 6 months ago

Hi @conscribtor , Thanks for your work. I think this change need more discussions, but i definitively think that you should ensure backward compatibility. We doesn't provide default configuration template for many reasons, most of them I don't know very well but you can check on olders PR / issues. People here can certainly give you much more information about the reasons of all theses changes. As you probably seen on our recent changes, we don't test pg =< 10 anymore. @tsoulabail Can you give us your opinion about this ?

conscribtor commented 6 months ago

Adding a new PostgreSQL version might be a good opportunity to fix those defaults. There might be ways to include them while remaining backwards compatible.

One possibility might be to version_compare('16', '>=') them, but use the annotations to reflect when they were originally changed upstream:

postgresql_vacuum_cost_page_miss:  "{{ 2 if postgresql_version is version_compare('16', '>=') else 10 }}" # (<= 13: 10, >= 14: 2) 0-10000 credits

Let me know what you think, if i should proceed with that idea and which I should include in that way.

tsoulabail commented 6 months ago

In 2023, the first work was to push pg14 and pg15, as quickly as possible, with in background "the backward compatibility ". I think that your proposal to cleanup is what needs to be done. I like the idea postgresql_vacuum_cost_page_miss: "{{ 2 if postgresql_version is version_compare('14', '>=') else 10 }}"

conscribtor commented 6 months ago

In 2023, the first work was to push pg14 and pg15, as quickly as possible, with in background "the backward compatibility ". I think that your proposal to cleanup is what needs to be done. I like the idea postgresql_vacuum_cost_page_miss: "{{ 2 if postgresql_version is version_compare('14', '>=') else 10 }}"

Thank you - I've pushed the new defaults using this pattern in 556219968c85e25dad218587014f5d2853f1b101.

MrMegaNova commented 5 months ago

I close this issue, work can be seen in #564