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

Enable archive_mode=always in Postgres 9.5 and 9.6 #254

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@pjungwir

pjungwir commented Jul 18, 2017

If the user sets postgresql_archive_mode: always then we should honor that in Postgres 9.5 and 9.6 (where it is supported), rather than just forcing it to on. This is important if you want the standby to also archive WAL, e.g. in a cascading standby setup or when running WAL-E from the standby.

@gclough

This seems like a sensible change.

@@ -206,7 +206,7 @@ checkpoint_warning = {{postgresql_checkpoint_warning}} # 0 disables
# - Archiving -
archive_mode = {{'on' if postgresql_archive_mode else 'off'}} # enables archiving; off, on, or always
archive_mode = {{('always' if postgresql_archive_mode == 'always' else 'on') if postgresql_archive_mode else 'off'}} # enables archiving; off, on, or always

This comment has been minimized.

@sebalix

sebalix Mar 26, 2018

Contributor

I prefer this simple syntax:
archive_mode = {{ postgresql_archive_mode }}
And document the different option values on/off/always in https://github.com/ANXS/postgresql/blob/master/defaults/main.yml#L261

@sebalix

sebalix Mar 26, 2018

Contributor

I prefer this simple syntax:
archive_mode = {{ postgresql_archive_mode }}
And document the different option values on/off/always in https://github.com/ANXS/postgresql/blob/master/defaults/main.yml#L261

This comment has been minimized.

@gclough

gclough Mar 26, 2018

Contributor

Yeah, that makes sense. There's no validation, but then we need to assume the users are sensible and in control. If it's not valid, then PostgreSQL will fail to startup and they will know quite quickly:

LOG:  invalid value for parameter "archive_mode": "typo_in_param"
HINT:  Available values: always, on, off.
< 2018-03-26 10:47:35.719 BST > FATAL:  configuration file "/var/lib/pgsql/9.6/data/postgresql.conf" contains errors
@gclough

gclough Mar 26, 2018

Contributor

Yeah, that makes sense. There's no validation, but then we need to assume the users are sensible and in control. If it's not valid, then PostgreSQL will fail to startup and they will know quite quickly:

LOG:  invalid value for parameter "archive_mode": "typo_in_param"
HINT:  Available values: always, on, off.
< 2018-03-26 10:47:35.719 BST > FATAL:  configuration file "/var/lib/pgsql/9.6/data/postgresql.conf" contains errors

This comment has been minimized.

@gclough

gclough Mar 26, 2018

Contributor

Oh... the only problem with the simple syntax, is that JINJA will always resolve "on/off" to "true/false", right?

We'll need to make sure it's handled just like synchronous_commit, as a string, not a boolean:

# Synchronization level:
# - off
# - local
# - remote_write
# - remote_apply (>= 9.6)
# - on
postgresql_synchronous_commit: "on"
@gclough

gclough Mar 26, 2018

Contributor

Oh... the only problem with the simple syntax, is that JINJA will always resolve "on/off" to "true/false", right?

We'll need to make sure it's handled just like synchronous_commit, as a string, not a boolean:

# Synchronization level:
# - off
# - local
# - remote_write
# - remote_apply (>= 9.6)
# - on
postgresql_synchronous_commit: "on"

This comment has been minimized.

@sebalix

sebalix Mar 26, 2018

Contributor

Ah... I remember a glitch about something like that yes. Well, we can accept this PR as-is, if someday PostgreSQL introduces a 4th value for this option, we will find another way to handle this :)

@sebalix

sebalix Mar 26, 2018

Contributor

Ah... I remember a glitch about something like that yes. Well, we can accept this PR as-is, if someday PostgreSQL introduces a 4th value for this option, we will find another way to handle this :)

This comment has been minimized.

@gclough

gclough Mar 26, 2018

Contributor

Actually, I think your way is cleaner @sebalix. The true/on, false/off syntax is used exclusively for parameters that have two values:

[ansible@localhost templates]$ grep ' if postgresql_' postgresql.conf-9.6.j2 | cut -f2 -d =
 {{'on' if postgresql_bonjour else 'off'}}				# advertise server via Bonjour
 {{'on' if postgresql_ssl else 'off'}}				# (change requires restart)
 {{ 'on' if postgresql_ssl_prefer_server_ciphers else 'off' }}		# (change requires restart)
 {{'on' if postgresql_password_encryption else 'off'}}
 {{'on' if postgresql_db_user_namespace else 'off'}}
 {{'on' if postgresql_row_security else 'off'}}
 {{'on' if postgresql_db_user_namespace else 'off'}}
 {{'on' if postgresql_fsync else 'off'}}				# flush data to disk for crash safety
 {{'on' if postgresql_full_page_writes else 'off'}}			# recover from partial page writes
 {{ 'on' if postgresql_wal_compression else 'off' }}
 {{ 'on' if postgresql_wal_log_hints else 'off' }}			# also do full page writes of non-critical updates
 {{ postgresql_max_wal_size if postgresql_max_wal_size or '1G' }}
 {{ postgresql_min_wal_size if postgresql_min_wal_size or '80MB' }}
 {{'on' if postgresql_archive_mode else 'off'}} # enables archiving; off, on, or always
 {{'on' if postgresql_track_commit_timestamp else 'off' }}
 '{{postgresql_synchronous_standby_num_sync}}{% if postgresql_synchronous_standby_names !
 {{'on' if postgresql_hot_standby else 'off'}}			# "on" allows queries during recovery
 {{'on' if postgresql_hot_standby_feedback or 'off'}}		# send info from standby to prevent
 {{'on' if postgresql_enable_bitmapscan else 'off'}}
 {{'on' if postgresql_enable_hashagg else 'off'}}
 {{'on' if postgresql_enable_hashjoin else 'off'}}
 {{'on' if postgresql_enable_indexscan else 'off'}}
 {{'on' if postgresql_enable_indexonlyscan else 'off'}}
 {{'on' if postgresql_enable_material else 'off'}}
 {{'on' if postgresql_enable_mergejoin else 'off'}}
 {{'on' if postgresql_enable_nestloop else 'off'}}
 {{'on' if postgresql_enable_seqscan else 'off'}}
 {{'on' if postgresql_enable_sort else 'off'}}
 {{'on' if postgresql_enable_tidscan else 'off'}}
 {{'on' if postgresql_enable_tidscan else 'off'}}
 {{'on' if postgresql_force_parallel_mode else 'off'}}
 {{'on' if postgresql_logging_collector else 'off'}}		# Enable capturing of stderr and csvlog
 {{'on' if postgresql_log_truncate_on_rotation else 'off'}}		# If on, an existing log file with the
 {{'on' if postgresql_syslog_sequence_numbers else 'off'}}
 {{'on' if postgresql_syslog_split_messages else 'off'}}
 {{'on' if postgresql_debug_print_parse else 'off'}}
 {{'on' if postgresql_debug_print_rewritten else 'off'}}
 {{'on' if postgresql_debug_print_plan else 'off'}}
 {{'on' if postgresql_debug_pretty_print else 'off'}}
 {{'on' if postgresql_log_checkpoints else 'off'}}
 {{'on' if postgresql_log_connections else 'off'}}
 {{'on' if postgresql_log_disconnections else 'off'}}
 {{'on' if postgresql_log_duration else 'off'}}
 {{'on' if postgresql_log_duration else 'off'}}
 {{'on' if postgresql_log_lock_waits else 'off'}}			# log lock waits >
 {{'on' if postgresql_track_activities else 'off'}}
 {{'on' if postgresql_track_counts else 'off'}}
 {{'on' if postgresql_track_io_timing else 'off'}}
 {{'on' if postgresql_update_process_title else 'off'}}
 {{'on' if postgresql_log_parser_stats else 'off'}}
 {{'on' if postgresql_log_planner_stats else 'off'}}
 {{'on' if postgresql_log_executor_stats else 'off'}}
 {{'on' if postgresql_log_statement_stats else 'off'}}
 {{'on' if postgresql_autovacuum else 'off'}}			# Enable autovacuum subprocess?  'on'
 {{'on' if postgresql_check_function_bodies else 'off'}}
 {{'on' if postgresql_default_transaction_read_only else 'off'}}
 {{'on' if postgresql_default_transaction_deferrable else 'off'}}
 {{'on' if postgresql_array_nulls else 'off'}}
 {{'on' if postgresql_default_with_oids else 'off'}}
 {{'on' if postgresql_escape_string_warning else 'off'}}
 {{'on' if postgresql_lo_compat_privileges else 'off'}}
 {{'on' if postgresql_quote_all_identifiers else 'off'}}
 {{'on' if postgresql_sql_inheritance else 'off'}}
 {{'on' if postgresql_standard_conforming_strings else 'off'}}
 {{'on' if postgresql_synchronize_seqscans else 'off'}}
 {{'on' if postgresql_transform_null_equals else 'off'}}
 {{'on' if postgresql_exit_on_error else 'off'}}			# terminate session on any error?
 {{'on' if postgresql_restart_after_crash else 'off'}}		# reinitialize after backend crash?

I think it's sensible to keep this syntax for boolean postgresql.conf parameters, and to use the other syntax you proposed for items with multiple options (enum). In v9.6, the archive_mode changed from bool -> enum... so it's sensible for us to also change.

We just need to be a little more careful about how we list the defaults, to ensure they are most obviously strings. For parameters such as wal_level, there are no quotes... but that's only OK, because on/off/true/false/yes/no aren't options. We don't get that luxury with archive_mode.

postgresql_wal_level: minimal # minimal, archive (<= 9.5), hot_standby (<= 9.5), replica (>= 9.6), or logical

@gclough

gclough Mar 26, 2018

Contributor

Actually, I think your way is cleaner @sebalix. The true/on, false/off syntax is used exclusively for parameters that have two values:

[ansible@localhost templates]$ grep ' if postgresql_' postgresql.conf-9.6.j2 | cut -f2 -d =
 {{'on' if postgresql_bonjour else 'off'}}				# advertise server via Bonjour
 {{'on' if postgresql_ssl else 'off'}}				# (change requires restart)
 {{ 'on' if postgresql_ssl_prefer_server_ciphers else 'off' }}		# (change requires restart)
 {{'on' if postgresql_password_encryption else 'off'}}
 {{'on' if postgresql_db_user_namespace else 'off'}}
 {{'on' if postgresql_row_security else 'off'}}
 {{'on' if postgresql_db_user_namespace else 'off'}}
 {{'on' if postgresql_fsync else 'off'}}				# flush data to disk for crash safety
 {{'on' if postgresql_full_page_writes else 'off'}}			# recover from partial page writes
 {{ 'on' if postgresql_wal_compression else 'off' }}
 {{ 'on' if postgresql_wal_log_hints else 'off' }}			# also do full page writes of non-critical updates
 {{ postgresql_max_wal_size if postgresql_max_wal_size or '1G' }}
 {{ postgresql_min_wal_size if postgresql_min_wal_size or '80MB' }}
 {{'on' if postgresql_archive_mode else 'off'}} # enables archiving; off, on, or always
 {{'on' if postgresql_track_commit_timestamp else 'off' }}
 '{{postgresql_synchronous_standby_num_sync}}{% if postgresql_synchronous_standby_names !
 {{'on' if postgresql_hot_standby else 'off'}}			# "on" allows queries during recovery
 {{'on' if postgresql_hot_standby_feedback or 'off'}}		# send info from standby to prevent
 {{'on' if postgresql_enable_bitmapscan else 'off'}}
 {{'on' if postgresql_enable_hashagg else 'off'}}
 {{'on' if postgresql_enable_hashjoin else 'off'}}
 {{'on' if postgresql_enable_indexscan else 'off'}}
 {{'on' if postgresql_enable_indexonlyscan else 'off'}}
 {{'on' if postgresql_enable_material else 'off'}}
 {{'on' if postgresql_enable_mergejoin else 'off'}}
 {{'on' if postgresql_enable_nestloop else 'off'}}
 {{'on' if postgresql_enable_seqscan else 'off'}}
 {{'on' if postgresql_enable_sort else 'off'}}
 {{'on' if postgresql_enable_tidscan else 'off'}}
 {{'on' if postgresql_enable_tidscan else 'off'}}
 {{'on' if postgresql_force_parallel_mode else 'off'}}
 {{'on' if postgresql_logging_collector else 'off'}}		# Enable capturing of stderr and csvlog
 {{'on' if postgresql_log_truncate_on_rotation else 'off'}}		# If on, an existing log file with the
 {{'on' if postgresql_syslog_sequence_numbers else 'off'}}
 {{'on' if postgresql_syslog_split_messages else 'off'}}
 {{'on' if postgresql_debug_print_parse else 'off'}}
 {{'on' if postgresql_debug_print_rewritten else 'off'}}
 {{'on' if postgresql_debug_print_plan else 'off'}}
 {{'on' if postgresql_debug_pretty_print else 'off'}}
 {{'on' if postgresql_log_checkpoints else 'off'}}
 {{'on' if postgresql_log_connections else 'off'}}
 {{'on' if postgresql_log_disconnections else 'off'}}
 {{'on' if postgresql_log_duration else 'off'}}
 {{'on' if postgresql_log_duration else 'off'}}
 {{'on' if postgresql_log_lock_waits else 'off'}}			# log lock waits >
 {{'on' if postgresql_track_activities else 'off'}}
 {{'on' if postgresql_track_counts else 'off'}}
 {{'on' if postgresql_track_io_timing else 'off'}}
 {{'on' if postgresql_update_process_title else 'off'}}
 {{'on' if postgresql_log_parser_stats else 'off'}}
 {{'on' if postgresql_log_planner_stats else 'off'}}
 {{'on' if postgresql_log_executor_stats else 'off'}}
 {{'on' if postgresql_log_statement_stats else 'off'}}
 {{'on' if postgresql_autovacuum else 'off'}}			# Enable autovacuum subprocess?  'on'
 {{'on' if postgresql_check_function_bodies else 'off'}}
 {{'on' if postgresql_default_transaction_read_only else 'off'}}
 {{'on' if postgresql_default_transaction_deferrable else 'off'}}
 {{'on' if postgresql_array_nulls else 'off'}}
 {{'on' if postgresql_default_with_oids else 'off'}}
 {{'on' if postgresql_escape_string_warning else 'off'}}
 {{'on' if postgresql_lo_compat_privileges else 'off'}}
 {{'on' if postgresql_quote_all_identifiers else 'off'}}
 {{'on' if postgresql_sql_inheritance else 'off'}}
 {{'on' if postgresql_standard_conforming_strings else 'off'}}
 {{'on' if postgresql_synchronize_seqscans else 'off'}}
 {{'on' if postgresql_transform_null_equals else 'off'}}
 {{'on' if postgresql_exit_on_error else 'off'}}			# terminate session on any error?
 {{'on' if postgresql_restart_after_crash else 'off'}}		# reinitialize after backend crash?

I think it's sensible to keep this syntax for boolean postgresql.conf parameters, and to use the other syntax you proposed for items with multiple options (enum). In v9.6, the archive_mode changed from bool -> enum... so it's sensible for us to also change.

We just need to be a little more careful about how we list the defaults, to ensure they are most obviously strings. For parameters such as wal_level, there are no quotes... but that's only OK, because on/off/true/false/yes/no aren't options. We don't get that luxury with archive_mode.

postgresql_wal_level: minimal # minimal, archive (<= 9.5), hot_standby (<= 9.5), replica (>= 9.6), or logical

This comment has been minimized.

@gclough

gclough Mar 26, 2018

Contributor

I've raised #318, because I think we need to tidy up things a little to prevent errors.

@gclough

gclough Mar 26, 2018

Contributor

I've raised #318, because I think we need to tidy up things a little to prevent errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment