Calculate Postgres parameters in the controller

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/config/config.go

@@ -22,9 +22,11 @@ func defaultFromEnv(value, key string) string {
 // FetchKeyCommand returns the fetch_key_cmd value stored in the encryption_key_command
 // variable used to enable TDE.
 func FetchKeyCommand(spec *v1beta1.PostgresClusterSpec) string {
-	if parameters := spec.Config.Parameters; parameters != nil {
-		if v, ok := parameters["encryption_key_command"]; ok {
-			return v.String()
+	if config := spec.Config; config != nil {
+		if parameters := config.Parameters; parameters != nil {
+			if v, ok := parameters["encryption_key_command"]; ok {
+				return v.String()
+			}
 		}
 	}
 

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)