When requested, load the "citus" library first

Issue: PGO-284
Issue: #3194

Author: cbandy

internal/patroni/config.go

@@ -227,17 +227,24 @@ func DynamicConfiguration(
 	// Override the above with mandatory parameters.
 	if pgParameters.Mandatory != nil {
 		for k, v := range pgParameters.Mandatory.AsMap() {
-			// Unlike other PostgreSQL parameters that have mandatory values,
-			// shared_preload_libraries is a comma separated list that can have
-			// other values appended in addition to the mandatory values. Below,
-			// any values provided in the CRD are appended after the mandatory
-			// values.
-			s, ok := parameters[k].(string)
-			if k == "shared_preload_libraries" && ok {
-				parameters[k] = v + "," + s
-			} else {
-				parameters[k] = v
+
+			// This parameter is a comma-separated list. Rather than overwrite the
+			// user-defined value, we want to combine it with the mandatory one.
+			// Some libraries belong at specific positions in the list, so figure
+			// that out as well.
+			if k == "shared_preload_libraries" {
+				// Load mandatory libraries ahead of user-defined libraries.
+				if s, ok := parameters[k].(string); ok && len(s) > 0 {
+					v = v + "," + s
+				}
+				// 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
+				if strings.Contains(v, "citus") {
+					v = "citus," + v
+				}
 			}
+
+			parameters[k] = v
 		}
 	}
 	postgresql["parameters"] = parameters

internal/patroni/config_test.go

@@ -394,7 +394,7 @@ func TestDynamicConfiguration(t *testing.T) {
 			},
 		},
 		{
-			name: "postgresql.parameters: mandatory shared_preload_libraries bad type",
+			name: "postgresql.parameters: mandatory shared_preload_libraries wrong-type is ignored",
 			input: map[string]any{
 				"postgresql": map[string]any{
 					"parameters": map[string]any{
@@ -420,6 +420,33 @@ func TestDynamicConfiguration(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "postgresql.parameters: shared_preload_libraries order",
+			input: map[string]any{
+				"postgresql": map[string]any{
+					"parameters": map[string]any{
+						"shared_preload_libraries": "given, citus, more",
+					},
+				},
+			},
+			params: postgres.Parameters{
+				Mandatory: parameters(map[string]string{
+					"shared_preload_libraries": "mandatory",
+				}),
+			},
+			expected: map[string]any{
+				"loop_wait": int32(10),
+				"ttl":       int32(30),
+				"postgresql": map[string]any{
+					"parameters": map[string]any{
+						"shared_preload_libraries": "citus,mandatory,given, citus, more",
+					},
+					"pg_hba":        []string{},
+					"use_pg_rewind": true,
+					"use_slots":     false,
+				},
+			},
+		},
 		{
 			name: "postgresql.pg_hba: wrong-type is ignored",
 			input: map[string]any{

internal/pgaudit/postgres.go

@@ -17,7 +17,6 @@ package pgaudit
 
 import (
 	"context"
-	"strings"
 
 	"github.com/crunchydata/postgres-operator/internal/logging"
 	"github.com/crunchydata/postgres-operator/internal/postgres"
@@ -67,7 +66,5 @@ func PostgreSQLParameters(outParameters *postgres.Parameters) {
 	// PostgreSQL must be restarted when changing this value.
 	// - https://github.com/pgaudit/pgaudit#settings
 	// - https://www.postgresql.org/docs/current/runtime-config-client.html
-	shared := outParameters.Mandatory.Value("shared_preload_libraries")
-	outParameters.Mandatory.Add("shared_preload_libraries",
-		strings.TrimPrefix(shared+",pgaudit", ","))
+	outParameters.Mandatory.AppendToList("shared_preload_libraries", "pgaudit")
 }

internal/pgmonitor/postgres.go

@@ -50,14 +50,7 @@ func PostgreSQLParameters(inCluster *v1beta1.PostgresCluster, outParameters *pos
 		// Exporter expects that shared_preload_libraries are installed
 		// pg_stat_statements: https://access.crunchydata.com/documentation/pgmonitor/latest/exporter/
 		// pgnodemx: https://github.com/CrunchyData/pgnodemx
-		libraries := []string{"pg_stat_statements", "pgnodemx"}
-
-		defined, found := outParameters.Mandatory.Get("shared_preload_libraries")
-		if found {
-			libraries = append(libraries, defined)
-		}
-
-		outParameters.Mandatory.Add("shared_preload_libraries", strings.Join(libraries, ","))
+		outParameters.Mandatory.AppendToList("shared_preload_libraries", "pg_stat_statements", "pgnodemx")
 		outParameters.Mandatory.Add("pgnodemx.kdapi_path",
 			postgres.DownwardAPIVolumeMount().MountPath)
 	}

internal/postgres/parameters.go

@@ -96,6 +96,22 @@ func (ps *ParameterSet) Add(name, value string) {
 	ps.values[ps.normalize(name)] = value
 }
 
+// AppendToList adds each value to the right-hand side of parameter name
+// as a comma-separated list without quoting.
+func (ps *ParameterSet) AppendToList(name string, value ...string) {
+	result := ps.Value(name)
+
+	if len(value) > 0 {
+		if len(result) > 0 {
+			result += "," + strings.Join(value, ",")
+		} else {
+			result = strings.Join(value, ",")
+		}
+	}
+
+	ps.Add(name, result)
+}
+
 // Get returns the value of parameter name and whether or not it was present in ps.
 func (ps ParameterSet) Get(name string) (string, bool) {
 	value, ok := ps.values[ps.normalize(name)]

internal/postgres/parameters_test.go

@@ -68,3 +68,26 @@ func TestParameterSet(t *testing.T) {
 	ps2.Add("x", "n")
 	assert.Assert(t, ps2.Value("x") != ps.Value("x"))
 }
+
+func TestParameterSetAppendToList(t *testing.T) {
+	ps := NewParameterSet()
+
+	ps.AppendToList("empty")
+	assert.Assert(t, ps.Has("empty"))
+	assert.Equal(t, ps.Value("empty"), "")
+
+	ps.AppendToList("empty")
+	assert.Equal(t, ps.Value("empty"), "", "expected no change")
+
+	ps.AppendToList("full", "a")
+	assert.Equal(t, ps.Value("full"), "a")
+
+	ps.AppendToList("full", "b")
+	assert.Equal(t, ps.Value("full"), "a,b")
+
+	ps.AppendToList("full")
+	assert.Equal(t, ps.Value("full"), "a,b", "expected no change")
+
+	ps.AppendToList("full", "a", "cd", `"e"`)
+	assert.Equal(t, ps.Value("full"), `a,b,a,cd,"e"`)
+}