Calculate Postgres parameters in the controller

The controller assigned mandatory and default values but did nothing
with values defined in the spec.

The patroni.DynamicConfiguration function was interpreting its
schemaless fields while also combining Postgres parameters from
elsewhere. Its tests were long and complicated.

 1. Postgres parameters are now extracted from the schemaless Patroni
    field in their own function with its own tests. Unusual types are
    handled more deliberately.

 2. The PostgresCluster controller now creates a single set of Postgres
    parameters based on all the fields of the PostgresCluster spec.

 3. The DynamicConfiguration function is simpler (44 lines, 30% smaller)
    and easier to test.

internal/collector/postgres.go

@@ -19,7 +19,7 @@ import (
 
 func NewConfigForPostgresPod(ctx context.Context,
 	inCluster *v1beta1.PostgresCluster,
-	outParameters *postgres.Parameters,
+	outParameters *postgres.ParameterSet,
 ) *Config {
 	config := NewConfig(inCluster.Spec.Instrumentation)
 
@@ -72,22 +72,22 @@ func EnablePostgresLogging(
 	ctx context.Context,
 	inCluster *v1beta1.PostgresCluster,
 	outConfig *Config,
-	outParameters *postgres.Parameters,
+	outParameters *postgres.ParameterSet,
 ) {
 	if feature.Enabled(ctx, feature.OpenTelemetryLogs) {
 		directory := postgres.LogDirectory()
 
 		// https://www.postgresql.org/docs/current/runtime-config-logging.html
-		outParameters.Mandatory.Add("logging_collector", "on")
-		outParameters.Mandatory.Add("log_directory", directory)
+		outParameters.Add("logging_collector", "on")
+		outParameters.Add("log_directory", directory)
 
 		// PostgreSQL v8.3 adds support for CSV logging, and
 		// PostgreSQL v15 adds support for JSON logging. The latter is preferred
 		// because newlines are escaped as "\n", U+005C + U+006E.
 		if inCluster.Spec.PostgresVersion < 15 {
-			outParameters.Mandatory.Add("log_destination", "csvlog")
+			outParameters.Add("log_destination", "csvlog")
 		} else {
-			outParameters.Mandatory.Add("log_destination", "jsonlog")
+			outParameters.Add("log_destination", "jsonlog")
 		}
 
 		// Keep seven days of logs named for the day of the week;
@@ -100,14 +100,14 @@ func EnablePostgresLogging(
 		// probably requires another process that deletes the oldest files.
 		//
 		// The ".log" suffix is replaced by ".json" for JSON log files.
-		outParameters.Mandatory.Add("log_filename", "postgresql-%a.log")
-		outParameters.Mandatory.Add("log_file_mode", "0660")
-		outParameters.Mandatory.Add("log_rotation_age", "1d")
-		outParameters.Mandatory.Add("log_rotation_size", "0")
-		outParameters.Mandatory.Add("log_truncate_on_rotation", "on")
+		outParameters.Add("log_filename", "postgresql-%a.log")
+		outParameters.Add("log_file_mode", "0660")
+		outParameters.Add("log_rotation_age", "1d")
+		outParameters.Add("log_rotation_size", "0")
+		outParameters.Add("log_truncate_on_rotation", "on")
 
 		// Log in a timezone that the OpenTelemetry Collector will understand.
-		outParameters.Mandatory.Add("log_timezone", "UTC")
+		outParameters.Add("log_timezone", "UTC")
 
 		// Keep track of what log records and files have been processed.
 		// Use a subdirectory of the logs directory to stay within the same failure domain.

internal/collector/postgres_test.go

@@ -27,9 +27,9 @@ func TestEnablePostgresLogging(t *testing.T) {
 		cluster.Spec.PostgresVersion = 99
 
 		config := NewConfig(nil)
-		params := postgres.NewParameters()
+		params := postgres.NewParameterSet()
 
-		EnablePostgresLogging(ctx, cluster, config, &params)
+		EnablePostgresLogging(ctx, cluster, config, params)
 
 		result, err := config.ToYAML()
 		assert.NilError(t, err)
@@ -255,9 +255,9 @@ service:
 		cluster.Spec.Instrumentation = testInstrumentationSpec()
 
 		config := NewConfig(cluster.Spec.Instrumentation)
-		params := postgres.NewParameters()
+		params := postgres.NewParameterSet()
 
-		EnablePostgresLogging(ctx, cluster, config, &params)
+		EnablePostgresLogging(ctx, cluster, config, params)
 
 		result, err := config.ToYAML()
 		assert.NilError(t, err)

internal/controller/postgrescluster/cluster.go

@@ -30,7 +30,7 @@ import (
 // files (etc) that apply to the entire cluster.
 func (r *Reconciler) reconcileClusterConfigMap(
 	ctx context.Context, cluster *v1beta1.PostgresCluster,
-	pgHBAs postgres.HBAs, pgParameters postgres.Parameters,
+	pgHBAs postgres.HBAs, pgParameters *postgres.ParameterSet,
 ) (*corev1.ConfigMap, error) {
 	clusterConfigMap := &corev1.ConfigMap{ObjectMeta: naming.ClusterConfigMap(cluster)}
 	clusterConfigMap.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ConfigMap"))

internal/controller/postgrescluster/controller.go

@@ -33,8 +33,6 @@ import (
 	"github.com/crunchydata/postgres-operator/internal/initialize"
 	"github.com/crunchydata/postgres-operator/internal/kubernetes"
 	"github.com/crunchydata/postgres-operator/internal/logging"
-	"github.com/crunchydata/postgres-operator/internal/pgaudit"
-	"github.com/crunchydata/postgres-operator/internal/pgbackrest"
 	"github.com/crunchydata/postgres-operator/internal/pgbouncer"
 	"github.com/crunchydata/postgres-operator/internal/pgmonitor"
 	"github.com/crunchydata/postgres-operator/internal/pki"
@@ -237,15 +235,9 @@ func (r *Reconciler) Reconcile(
 	pgmonitor.PostgreSQLHBAs(ctx, cluster, &pgHBAs)
 	pgbouncer.PostgreSQL(cluster, &pgHBAs)
 
-	pgParameters := postgres.NewParameters()
-	pgaudit.PostgreSQLParameters(&pgParameters)
-	pgbackrest.PostgreSQL(cluster, &pgParameters, backupsSpecFound)
-	pgmonitor.PostgreSQLParameters(ctx, cluster, &pgParameters)
+	pgParameters := r.generatePostgresParameters(ctx, cluster, backupsSpecFound)
 
-	otelConfig := collector.NewConfigForPostgresPod(ctx, cluster, &pgParameters)
-
-	// Set huge_pages = try if a hugepages resource limit > 0, otherwise set "off"
-	postgres.SetHugePages(cluster, &pgParameters)
+	otelConfig := collector.NewConfigForPostgresPod(ctx, cluster, pgParameters)
 
 	if err == nil {
 		rootCA, err = r.reconcileRootCertificate(ctx, cluster)

internal/controller/postgrescluster/patroni.go

@@ -173,7 +173,7 @@ func (r *Reconciler) reconcilePatroniDistributedConfiguration(
 
 func (r *Reconciler) reconcilePatroniDynamicConfiguration(
 	ctx context.Context, cluster *v1beta1.PostgresCluster, instances *observedInstances,
-	pgHBAs postgres.HBAs, pgParameters postgres.Parameters,
+	pgHBAs postgres.HBAs, pgParameters *postgres.ParameterSet,
 ) error {
 	if !patroni.ClusterBootstrapped(cluster) {
 		// Patroni has not yet bootstrapped. Dynamic configuration happens through

internal/controller/postgrescluster/postgres.go

@@ -6,6 +6,7 @@ package postgrescluster
 
 import (
 	"bytes"
+	"cmp"
 	"context"
 	"fmt"
 	"io"
@@ -29,14 +30,78 @@ import (
 	"github.com/crunchydata/postgres-operator/internal/initialize"
 	"github.com/crunchydata/postgres-operator/internal/logging"
 	"github.com/crunchydata/postgres-operator/internal/naming"
+	"github.com/crunchydata/postgres-operator/internal/patroni"
 	"github.com/crunchydata/postgres-operator/internal/pgaudit"
+	"github.com/crunchydata/postgres-operator/internal/pgbackrest"
+	"github.com/crunchydata/postgres-operator/internal/pgmonitor"
 	"github.com/crunchydata/postgres-operator/internal/postgis"
 	"github.com/crunchydata/postgres-operator/internal/postgres"
 	pgpassword "github.com/crunchydata/postgres-operator/internal/postgres/password"
 	"github.com/crunchydata/postgres-operator/internal/util"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
+// generatePostgresParameters produces the parameter set for cluster that
+// incorporates, from highest to lowest precedence:
+//  1. mandatory values determined by controllers
+//  2. parameters in cluster.spec.config.parameters
+//  3. parameters in cluster.spec.patroni.dynamicConfiguration
+//  4. default values determined by contollers
+func (*Reconciler) generatePostgresParameters(
+	ctx context.Context, cluster *v1beta1.PostgresCluster, backupsSpecFound bool,
+) *postgres.ParameterSet {
+	builtin := postgres.NewParameters()
+	pgaudit.PostgreSQLParameters(&builtin)
+	pgbackrest.PostgreSQL(cluster, &builtin, backupsSpecFound)
+	pgmonitor.PostgreSQLParameters(ctx, cluster, &builtin)
+	postgres.SetHugePages(cluster, &builtin)
+
+	// Last write wins, so start with the recommended defaults.
+	result := cmp.Or(builtin.Default.DeepCopy(), postgres.NewParameterSet())
+
+	// Overwrite the above with any parameters specified in the Patroni section.
+	for k, v := range patroni.PostgresParameters(cluster.Spec.Patroni).AsMap() {
+		result.Add(k, v)
+	}
+
+	// Overwrite the above with any parameters specified in the Config section.
+	if config := cluster.Spec.Config; config != nil {
+		for k, v := range config.Parameters {
+			result.Add(k, v.String())
+		}
+	}
+
+	// Overwrite the above with mandatory values.
+	if builtin.Mandatory != nil {
+		// This parameter is a comma-separated list. Rather than overwrite the
+		// user-defined value, we want to combine it with the mandatory one.
+		preload := result.Value("shared_preload_libraries")
+
+		for k, v := range builtin.Mandatory.AsMap() {
+			// Load mandatory libraries ahead of user-defined libraries.
+			if k == "shared_preload_libraries" && len(v) > 0 && len(preload) > 0 {
+				v = v + "," + preload
+			}
+
+			result.Add(k, v)
+		}
+	}
+
+	// Some preload libraries belong at specific positions in this list.
+	if preload, ok := result.Get("shared_preload_libraries"); ok {
+		// Load "citus" ahead of any other libraries.
+		// - https://github.com/citusdata/citus/blob/v12.0.0/src/backend/distributed/shared_library_init.c#L417-L419
+		// - https://github.com/citusdata/citus/blob/v13.0.0/src/backend/distributed/shared_library_init.c#L420-L422
+		if strings.Contains(preload, "citus") {
+			preload = "citus," + preload
+		}
+
+		result.Add("shared_preload_libraries", preload)
+	}
+
+	return result
+}
+
 // generatePostgresUserSecret returns a Secret containing a password and
 // connection details for the first database in spec. When existing is nil or
 // lacks a password or verifier, a new password and verifier are generated.

internal/controller/postgrescluster/postgres_test.go

@@ -34,6 +34,123 @@ import (
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
+func TestGeneratePostgresParameters(t *testing.T) {
+	ctx := context.Background()
+	reconciler := &Reconciler{}
+
+	builtin := reconciler.generatePostgresParameters(ctx, v1beta1.NewPostgresCluster(), false)
+	assert.Assert(t, len(builtin.AsMap()) > 0,
+		"expected an empty cluster to have some builtin parameters")
+
+	assert.Equal(t, builtin.Value("jit"), "off",
+		"BUG IN TEST: expected JIT to be disabled")
+
+	assert.Equal(t, builtin.Value("shared_preload_libraries"), "pgaudit",
+		"BUG IN TEST: expected pgAudit to be mandatory")
+
+	t.Run("Config", func(t *testing.T) {
+		cluster := v1beta1.NewPostgresCluster()
+		require.UnmarshalInto(t, &cluster.Spec.Config, `{
+			parameters: {
+				something: str,
+				another: 5,
+			},
+		}`)
+
+		result := reconciler.generatePostgresParameters(ctx, cluster, false)
+		assert.Assert(t, cmp.LenMap(result.AsMap(), len(builtin.AsMap())+2),
+			"expected two parameters from the Config section")
+
+		assert.Equal(t, result.Value("another"), "5")
+		assert.Equal(t, result.Value("something"), "str")
+	})
+
+	t.Run("Patroni", func(t *testing.T) {
+		cluster := v1beta1.NewPostgresCluster()
+		require.UnmarshalInto(t, &cluster.Spec.Patroni, `{
+			dynamicConfiguration: {
+				postgresql: { parameters: {
+					something: str,
+					another: 5.1,
+				} },
+			},
+		}`)
+
+		result := reconciler.generatePostgresParameters(ctx, cluster, false)
+		assert.Assert(t, cmp.LenMap(result.AsMap(), len(builtin.AsMap())+2),
+			"expected two parameters from the Patroni section")
+
+		assert.Equal(t, result.Value("another"), "5.1")
+		assert.Equal(t, result.Value("something"), "str")
+	})
+
+	t.Run("Precedence", func(t *testing.T) {
+		cluster := v1beta1.NewPostgresCluster()
+		require.UnmarshalInto(t, &cluster.Spec.Config, `{
+			parameters: {
+				something: replaced,
+				unrelated: used,
+				jit: "on",
+			},
+		}`)
+		require.UnmarshalInto(t, &cluster.Spec.Patroni, `{
+			dynamicConfiguration: {
+				postgresql: { parameters: {
+					something: str,
+					another: 5.1,
+				} },
+			},
+		}`)
+
+		result := reconciler.generatePostgresParameters(ctx, cluster, false)
+		assert.Assert(t, cmp.LenMap(result.AsMap(), len(builtin.AsMap())+3+1-1),
+			"expected three parameters from the Config section,"+
+				"plus one from the Patroni section, minus one default")
+
+		assert.Equal(t, result.Value("another"), "5.1")        // Patroni
+		assert.Equal(t, result.Value("something"), "replaced") // Config
+		assert.Equal(t, result.Value("unrelated"), "used")     // Config
+		assert.Equal(t, result.Value("jit"), "on")             // Config
+	})
+
+	t.Run("shared_preload_libraries", func(t *testing.T) {
+		t.Run("NumericIncluded", func(t *testing.T) {
+			cluster := v1beta1.NewPostgresCluster()
+			require.UnmarshalInto(t, &cluster.Spec.Config, `{
+				parameters: {
+					shared_preload_libraries: 123,
+				},
+			}`)
+
+			result := reconciler.generatePostgresParameters(ctx, cluster, false)
+			assert.Assert(t, cmp.Contains(result.Value("shared_preload_libraries"), "123"))
+		})
+
+		t.Run("Precedence", func(t *testing.T) {
+			cluster := v1beta1.NewPostgresCluster()
+			require.UnmarshalInto(t, &cluster.Spec.Config, `{
+				parameters: {
+					shared_preload_libraries: given,
+				},
+			}`)
+
+			result := reconciler.generatePostgresParameters(ctx, cluster, false)
+			assert.Equal(t, result.Value("shared_preload_libraries"), "pgaudit,given",
+				"expected mandatory ahead of specified")
+
+			require.UnmarshalInto(t, &cluster.Spec.Config, `{
+				parameters: {
+					shared_preload_libraries: 'given, citus,other'
+				},
+			}`)
+
+			result = reconciler.generatePostgresParameters(ctx, cluster, false)
+			assert.Equal(t, result.Value("shared_preload_libraries"), "citus,pgaudit,given, citus,other",
+				"expected citus in front")
+		})
+	})
+}
+
 func TestGeneratePostgresUserSecret(t *testing.T) {
 	_, tClient := setupKubernetes(t)
 	require.ParallelCapacity(t, 0)