PR feedback (including the feedback from canonical/postgresql-k8s-ope…

…rator#281)

config.yaml

@@ -2,75 +2,103 @@
 # See LICENSE file for licensing details.
 
 options:
-  connection_ssl:
-    description: |
-      Enables SSL connections
-    type: boolean
   durability_synchronous_commit:
     description: |
-      Sets the current transactions synchronization level
+      Sets the current transactions synchronization level. This charm allows only the
+      “on”, “remote_apply” and “remote_write” values to avoid data loss if the primary
+      crashes and there are replicas.
     type: string
+    default: "on"
   instance_default_text_search_config:
     description: |
       Selects the text search configuration that is used by those variants of the text
-      search functions that do not have an explicit argument specifying it
+      search functions that do not have an explicit argument specifying it.
+      Allowed values start with “pg_catalog.” followed by a language name, like
+      “pg_catalog.english”.
     type: string
+    default: "pg_catalog.simple"
   instance_password_encryption:
     description: |
-      Determines the algorithm to use to encrypt the password
+      Determines the algorithm to use to encrypt the password.
+      Allowed values are: “md5” and “scram-sha-256”.
     type: string
+    default: "scram-sha-256"
   logging_log_connections:
     description: |
-      Logs each successful connection
+      Logs each successful connection.
     type: boolean
+    default: false
   logging_log_disconnections:
     description: |
-      Logs end of a session, including duration
+      Logs end of a session, including duration.
     type: boolean
+    default: false
   logging_log_lock_waits:
     description: |
-      Logs long lock waits
+      Logs long lock waits.
     type: boolean
+    default: false
   logging_log_min_duration_statement:
     description: |
-      Sets the minimum running time above which statements will be logged
+      Sets the minimum running time (milliseconds) above which statements will be logged.
+      Allowed values are: from -1 to 2147483647 (-1 disables logging
+      statement durations).
     type: int
+    default: -1
   memory_maintenance_work_mem:
     description: |
-      Sets the maximum memory to be used for maintenance operations
+      Sets the maximum memory (KB) to be used for maintenance operations.
+      Allowed values are: from 1024 to 2147483647.
     type: int
+    default: 65536
   memory_max_prepared_transactions:
     description: |
-      Sets the maximum number of simultaneously prepared transactions
+      Sets the maximum number of simultaneously prepared transactions.
+      Allowed values are: from 0 to 262143.
     type: int
+    default: 0
   memory_shared_buffers:
     description: |
-      Sets the number of shared memory buffers used by the server
+      Sets the number of shared memory buffers (8 kB) used by the server. This charm allows
+      to set this value up to 40% of the available memory from the unit, as it is unlikely
+      that an allocation of more than that will work better than a smaller amount.
+      Allowed values are: from 16 to 1073741823.
     type: int
   memory_temp_buffers:
     description: |
-      Sets the maximum number of temporary buffers used by each session
+      Sets the maximum number of temporary buffers (8 kB) used by each session.
+      Allowed values are: from 100 to 1073741823.
     type: int
+    default: 1024
   memory_work_mem:
     description: |
-      Sets the maximum memory to be used for query workspaces
+      Sets the maximum memory (KB) to be used for query workspaces.
+      Allowed values are: from 64 to 2147483647.
     type: int
+    default: 4096
   optimizer_constraint_exclusion:
     description: |
-      Enables the planner to use constraints to optimize queries
+      Enables the planner to use constraints to optimize queries.
+      Allowed values are: “on”, “off” and “partition”.
     type: string
+    default: "partition"
   optimizer_default_statistics_target:
     description: |
-      Sets the default statistics target
+      Sets the default statistics target. Allowed values are: from 1 to 10000.
     type: int
+    default: 100
   optimizer_from_collapse_limit:
     description: |
-      Sets the FROM-list size beyond which subqueries are not collapsed
+      Sets the FROM-list size beyond which subqueries are not collapsed.
+      Allowed values are: from 1 to 2147483647.
     type: int
+    default: 8
   optimizer_join_collapse_limit:
     description: |
-      Sets the FROM-list size beyond which JOIN constructs are not flattened
+      Sets the FROM-list size beyond which JOIN constructs are not flattened.
+      Allowed values are: from 1 to 2147483647.
     type: int
+    default: 8
   plugin_citext_enable:
     default: false
     type: boolean
@@ -111,57 +139,82 @@ options:
       Only comes into effect when the `production` profile is selected.
   request_date_style:
     description: |
-      Sets the display format for date and time values
+      Sets the display format for date and time values. Allowed formats are explained
+      in https://www.postgresql.org/docs/14/runtime-config-client.html#GUC-DATESTYLE.
     type: string
+    default: "ISO, MDY"
   request_standard_conforming_strings:
     description: |
-      Causes ... strings to treat backslashes literally
+      Causes ... strings to treat backslashes literally.
     type: boolean
+    default: true
   request_time_zone:
     description: |
-      Sets the time zone for displaying and interpreting time stamps
+      Sets the time zone for displaying and interpreting time stamps.
+      Allowed values are the ones from IANA time zone data, a time zone abbreviation
+      like PST and POSIX-style time zone specifications.
     type: string
+    default: "UTC"
   response_bytea_output:
     description: |
-      Sets the output format for bytes
+      Sets the output format for bytes.
+      Allowed values are: “escape” and “hex”.
     type: string
+    default: "hex"
   response_lc_monetary:
     description: |
-      Sets the locale for formatting monetary amounts
+      Sets the locale for formatting monetary amounts.
+      Allowed values are the locales available in the unit.
     type: string
+    default: "C"
   response_lc_numeric:
     description: |
-      Sets the locale for formatting numbers
+      Sets the locale for formatting numbers.
+      Allowed values are the locales available in the unit.
     type: string
+    default: "C"
   response_lc_time:
     description: |
-      Sets the locale for formatting date and time values
+      Sets the locale for formatting date and time values.
+      Allowed values are the locales available in the unit.
     type: string
+    default: "C"
   vacuum_autovacuum_analyze_scale_factor:
     description: |
       Specifies a fraction of the table size to add to autovacuum_vacuum_threshold when
-      deciding whether to trigger a VACUUM
+      deciding whether to trigger a VACUUM. The default, 0.1, means 10% of table size.
+      Allowed values are: from 0 to 100.
     type: float
+    default: 0.1
   vacuum_autovacuum_analyze_threshold:
     description: |
       Sets the minimum number of inserted, updated or deleted tuples needed to trigger
-      an ANALYZE in any one table.
+      an ANALYZE in any one table. Allowed values are: from 0 to 2147483647.
     type: int
+    default: 50
   vacuum_autovacuum_freeze_max_age:
     description: |
       Maximum age (in transactions) before triggering autovacuum on a table to prevent
-      transaction ID wraparound
+      transaction ID wraparound. Allowed values are: from 100000 to 2000000000.
     type: int
+    default: 200000000
   vacuum_autovacuum_vacuum_cost_delay:
     description: |
-      Sets cost delay value (milliseconds) that will be used in automatic VACUUM operations
+      Sets cost delay value (milliseconds) that will be used in automatic VACUUM operations.
+      Allowed values are: from -1 to 100 (-1 tells PostgreSQL to use the regular
+      vacuum_cost_delay value).
     type: float
+    default: 2.0
   vacuum_autovacuum_vacuum_scale_factor:
     description: |
       Specifies a fraction of the table size to add to autovacuum_vacuum_threshold when
-      deciding whether to trigger a VACUUM
+      deciding whether to trigger a VACUUM. The default, 0.2, means 20% of table size.
+      Allowed values are: from 0 to 100.
     type: float
+    default: 0.2
   vacuum_vacuum_freeze_table_age:
     description: |
-      Age at which VACUUM should scan whole table to freeze tuples
+      Age (in transactions) at which VACUUM should scan whole table to freeze tuples.
+      Allowed values are: from 0 to 2000000000.
     type: int
+    default: 150000000

src/charm.py

@@ -724,9 +724,7 @@ def is_primary(self) -> bool:
     @property
     def is_tls_enabled(self) -> bool:
         """Return whether TLS is enabled."""
-        return all(self.tls.get_tls_files()) and (
-            self.config.connection_ssl or self.config.connection_ssl is None
-        )
+        return all(self.tls.get_tls_files())
 
     @property
     def _peer_members_ips(self) -> Set[str]:
@@ -1490,11 +1488,6 @@ def update_config(self, is_creating_backup: bool = False) -> bool:
 
     def _validate_config_options(self) -> None:
         """Validates specific config options that need access to the database or to the TLS status."""
-        if self.config.connection_ssl and not self.is_tls_enabled:
-            raise Exception(
-                "connection_ssl config option should not be set to true when there is no configured TLS relation"
-            )
-
         if (
             self.config.instance_default_text_search_config is not None
             and self.config.instance_default_text_search_config

src/config.py

@@ -16,7 +16,6 @@
 class CharmConfig(BaseConfigModel):
     """Manager for the structured configuration."""
 
-    connection_ssl: Optional[bool]
     durability_synchronous_commit: Optional[str]
     instance_default_text_search_config: Optional[str]
     instance_password_encryption: Optional[str]
@@ -114,7 +113,7 @@ def memory_max_prepared_transactions_values(cls, value: int) -> Optional[int]:
     @classmethod
     def memory_shared_buffers_values(cls, value: int) -> Optional[int]:
         """Check memory_shared_buffers config option is greater or equal than 16."""
-        if value < 16:
+        if value < 16 or value > 1073741823:
             raise ValueError("Shared buffers config option should be at least 16")
 
         return value
@@ -202,6 +201,7 @@ def response_lc_values(cls, value: str) -> Optional[str]:
         """Check if the requested locale is available in the system."""
         output = subprocess.check_output(["ls", "/snap/charmed-postgresql/current/usr/lib/locale"])
         locales = [locale.decode() for locale in output.splitlines()]
+        locales.append("C")
         if value not in locales:
             raise ValueError("Value not one of the locales available in the system")
 

tests/integration/test_charm.py

@@ -126,12 +126,12 @@ async def test_settings_are_correct(ops_test: OpsTest, unit_id: int):
     # Validate each configuration set by Patroni on PostgreSQL.
     assert settings["archive_command"] == "/bin/true"
     assert settings["archive_mode"] == "on"
-    assert settings["autovacuum"]
+    assert settings["autovacuum"] == "on"
     assert settings["cluster_name"] == DATABASE_APP_NAME
     assert settings["data_directory"] == f"{STORAGE_PATH}/var/lib/postgresql"
     assert settings["data_checksums"] == "on"
-    assert settings["fsync"]
-    assert settings["full_page_writes"]
+    assert settings["fsync"] == "on"
+    assert settings["full_page_writes"] == "on"
     assert settings["lc_messages"] == "en_US.UTF8"
     assert settings["listen_addresses"] == host
     assert settings["log_autovacuum_min_duration"] == "60000"
@@ -147,9 +147,9 @@ async def test_settings_are_correct(ops_test: OpsTest, unit_id: int):
     settings = result.json()
 
     # Validate each configuration related to Patroni
-    assert settings["postgresql"]["use_pg_rewind"]
-    assert settings["postgresql"]["remove_data_directory_on_rewind_failure"]
-    assert settings["postgresql"]["remove_data_directory_on_diverged_timelines"]
+    assert settings["postgresql"]["use_pg_rewind"] is True
+    assert settings["postgresql"]["remove_data_directory_on_rewind_failure"] is True
+    assert settings["postgresql"]["remove_data_directory_on_diverged_timelines"] is True
     assert settings["loop_wait"] == 10
     assert settings["retry_timeout"] == 10
     assert settings["maximum_lag_on_failover"] == 1048576

tests/integration/test_tls.py

@@ -140,30 +140,7 @@ async def test_tls_enabled(ops_test: OpsTest) -> None:
             "grep 'connection authorized: user=rewind database=postgres SSL enabled' /var/snap/charmed-postgresql/common/var/log/postgresql/postgresql-*.log",
         )
 
-        # Disable TLS.
-        logger.info("disabling TLS through connection_ssl config option")
-        await ops_test.model.applications[DATABASE_APP_NAME].set_config(
-            {"connection_ssl": "False"}
-        )
-        await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active")
-        for unit in ops_test.model.applications[DATABASE_APP_NAME].units:
-            assert await check_tls(ops_test, unit.name, enabled=False)
-            assert await check_tls_patroni_api(ops_test, unit.name, enabled=False)
-
-        # Re-enable TLS.
-        logger.info("re-enabling TLS through connection_ssl config option")
-        await ops_test.model.applications[DATABASE_APP_NAME].set_config({"connection_ssl": "True"})
-        await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active")
-        for unit in ops_test.model.applications[DATABASE_APP_NAME].units:
-            assert await check_tls(ops_test, unit.name, enabled=True)
-            assert await check_tls_patroni_api(ops_test, unit.name, enabled=True)
-
         # Remove the relation.
-        logger.info("removing the TLS relation")
-        await ops_test.model.applications[DATABASE_APP_NAME].set_config(
-            {"connection_ssl": "False"}
-        )
-        await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active")
         await ops_test.model.applications[DATABASE_APP_NAME].remove_relation(
             f"{DATABASE_APP_NAME}:certificates", f"{TLS_CERTIFICATES_APP_NAME}:certificates"
         )

tests/unit/test_charm.py

@@ -250,7 +250,8 @@ def test_on_config_changed(
         _enable_disable_extensions.assert_called_once()
         _set_up_relation.assert_called_once()
 
-    def test_enable_disable_extensions(self):
+    @patch("subprocess.check_output", return_value=b"C")
+    def test_enable_disable_extensions(self, _):
         with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock:
             # Test when all extensions install/uninstall succeed.
             postgresql_mock.enable_disable_extension.side_effect = None
@@ -457,6 +458,7 @@ def test_on_start_replica(
         self.assertTrue(isinstance(self.harness.model.unit.status, WaitingStatus))
 
     @patch_network_get(private_address="1.1.1.1")
+    @patch("subprocess.check_output", return_value=b"C")
     @patch("charm.snap.SnapCache")
     @patch("charm.PostgresqlOperatorCharm.postgresql")
     @patch("charm.Patroni")
@@ -471,6 +473,7 @@ def test_on_start_no_patroni_member(
         patroni,
         _postgresql,
         _snap_cache,
+        _,
     ):
         # Mock the passwords.
         patroni.return_value.member_started = False
@@ -1055,6 +1058,7 @@ def test_restart(self, _are_all_members_ready, _restart_postgresql):
         mock_event.defer.assert_not_called()
 
     @patch_network_get(private_address="1.1.1.1")
+    @patch("subprocess.check_output", return_value=b"C")
     @patch("charm.snap.SnapCache")
     @patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_acquire_lock")
     @patch("charm.Patroni.reload_patroni_configuration")
@@ -1075,6 +1079,7 @@ def test_update_config(
         _reload_patroni_configuration,
         _restart,
         ___,
+        ____,
     ):
         with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock:
             # Mock some properties.

tests/unit/test_db.py

@@ -182,6 +182,7 @@ def test_get_extensions(self):
             ([extensions[1], extensions[2]], {extensions[2]}),
         )
 
+    @patch("subprocess.check_output", return_value=b"C")
     @patch("relations.db.DbProvides._update_unit_status")
     @patch("relations.db.DbProvides.update_endpoints")
     @patch("charm.PostgresqlOperatorCharm.enable_disable_extensions")
@@ -194,6 +195,7 @@ def test_set_up_relation(
         _enable_disable_extensions,
         _update_endpoints,
         _update_unit_status,
+        _,
     ):
         with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock:
             # Define some mocks' side effects.