Skip to content

Commit 2a2fe9b

Browse files
committed
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.
1 parent 6ba9057 commit 2a2fe9b

File tree

17 files changed

+439
-354
lines changed

17 files changed

+439
-354
lines changed

internal/collector/postgres.go

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import (
1919

2020
func NewConfigForPostgresPod(ctx context.Context,
2121
inCluster *v1beta1.PostgresCluster,
22-
outParameters *postgres.Parameters,
22+
outParameters *postgres.ParameterSet,
2323
) *Config {
2424
config := NewConfig(inCluster.Spec.Instrumentation)
2525

@@ -72,22 +72,22 @@ func EnablePostgresLogging(
7272
ctx context.Context,
7373
inCluster *v1beta1.PostgresCluster,
7474
outConfig *Config,
75-
outParameters *postgres.Parameters,
75+
outParameters *postgres.ParameterSet,
7676
) {
7777
if feature.Enabled(ctx, feature.OpenTelemetryLogs) {
7878
directory := postgres.LogDirectory()
7979

8080
// https://www.postgresql.org/docs/current/runtime-config-logging.html
81-
outParameters.Mandatory.Add("logging_collector", "on")
82-
outParameters.Mandatory.Add("log_directory", directory)
81+
outParameters.Add("logging_collector", "on")
82+
outParameters.Add("log_directory", directory)
8383

8484
// PostgreSQL v8.3 adds support for CSV logging, and
8585
// PostgreSQL v15 adds support for JSON logging. The latter is preferred
8686
// because newlines are escaped as "\n", U+005C + U+006E.
8787
if inCluster.Spec.PostgresVersion < 15 {
88-
outParameters.Mandatory.Add("log_destination", "csvlog")
88+
outParameters.Add("log_destination", "csvlog")
8989
} else {
90-
outParameters.Mandatory.Add("log_destination", "jsonlog")
90+
outParameters.Add("log_destination", "jsonlog")
9191
}
9292

9393
// Keep seven days of logs named for the day of the week;
@@ -100,14 +100,14 @@ func EnablePostgresLogging(
100100
// probably requires another process that deletes the oldest files.
101101
//
102102
// The ".log" suffix is replaced by ".json" for JSON log files.
103-
outParameters.Mandatory.Add("log_filename", "postgresql-%a.log")
104-
outParameters.Mandatory.Add("log_file_mode", "0660")
105-
outParameters.Mandatory.Add("log_rotation_age", "1d")
106-
outParameters.Mandatory.Add("log_rotation_size", "0")
107-
outParameters.Mandatory.Add("log_truncate_on_rotation", "on")
103+
outParameters.Add("log_filename", "postgresql-%a.log")
104+
outParameters.Add("log_file_mode", "0660")
105+
outParameters.Add("log_rotation_age", "1d")
106+
outParameters.Add("log_rotation_size", "0")
107+
outParameters.Add("log_truncate_on_rotation", "on")
108108

109109
// Log in a timezone that the OpenTelemetry Collector will understand.
110-
outParameters.Mandatory.Add("log_timezone", "UTC")
110+
outParameters.Add("log_timezone", "UTC")
111111

112112
// Keep track of what log records and files have been processed.
113113
// Use a subdirectory of the logs directory to stay within the same failure domain.

internal/collector/postgres_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,9 @@ func TestEnablePostgresLogging(t *testing.T) {
2727
cluster.Spec.PostgresVersion = 99
2828

2929
config := NewConfig(nil)
30-
params := postgres.NewParameters()
30+
params := postgres.NewParameterSet()
3131

32-
EnablePostgresLogging(ctx, cluster, config, &params)
32+
EnablePostgresLogging(ctx, cluster, config, params)
3333

3434
result, err := config.ToYAML()
3535
assert.NilError(t, err)
@@ -255,9 +255,9 @@ service:
255255
cluster.Spec.Instrumentation = testInstrumentationSpec()
256256

257257
config := NewConfig(cluster.Spec.Instrumentation)
258-
params := postgres.NewParameters()
258+
params := postgres.NewParameterSet()
259259

260-
EnablePostgresLogging(ctx, cluster, config, &params)
260+
EnablePostgresLogging(ctx, cluster, config, params)
261261

262262
result, err := config.ToYAML()
263263
assert.NilError(t, err)

internal/controller/postgrescluster/cluster.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import (
3030
// files (etc) that apply to the entire cluster.
3131
func (r *Reconciler) reconcileClusterConfigMap(
3232
ctx context.Context, cluster *v1beta1.PostgresCluster,
33-
pgHBAs postgres.HBAs, pgParameters postgres.Parameters,
33+
pgHBAs postgres.HBAs, pgParameters *postgres.ParameterSet,
3434
) (*corev1.ConfigMap, error) {
3535
clusterConfigMap := &corev1.ConfigMap{ObjectMeta: naming.ClusterConfigMap(cluster)}
3636
clusterConfigMap.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ConfigMap"))

internal/controller/postgrescluster/controller.go

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,6 @@ import (
3333
"github.com/crunchydata/postgres-operator/internal/initialize"
3434
"github.com/crunchydata/postgres-operator/internal/kubernetes"
3535
"github.com/crunchydata/postgres-operator/internal/logging"
36-
"github.com/crunchydata/postgres-operator/internal/pgaudit"
37-
"github.com/crunchydata/postgres-operator/internal/pgbackrest"
3836
"github.com/crunchydata/postgres-operator/internal/pgbouncer"
3937
"github.com/crunchydata/postgres-operator/internal/pgmonitor"
4038
"github.com/crunchydata/postgres-operator/internal/pki"
@@ -237,15 +235,9 @@ func (r *Reconciler) Reconcile(
237235
pgmonitor.PostgreSQLHBAs(ctx, cluster, &pgHBAs)
238236
pgbouncer.PostgreSQL(cluster, &pgHBAs)
239237

240-
pgParameters := postgres.NewParameters()
241-
pgaudit.PostgreSQLParameters(&pgParameters)
242-
pgbackrest.PostgreSQL(cluster, &pgParameters, backupsSpecFound)
243-
pgmonitor.PostgreSQLParameters(ctx, cluster, &pgParameters)
238+
pgParameters := r.generatePostgresParameters(ctx, cluster, backupsSpecFound)
244239

245-
otelConfig := collector.NewConfigForPostgresPod(ctx, cluster, &pgParameters)
246-
247-
// Set huge_pages = try if a hugepages resource limit > 0, otherwise set "off"
248-
postgres.SetHugePages(cluster, &pgParameters)
240+
otelConfig := collector.NewConfigForPostgresPod(ctx, cluster, pgParameters)
249241

250242
if err == nil {
251243
rootCA, err = r.reconcileRootCertificate(ctx, cluster)

internal/controller/postgrescluster/patroni.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ func (r *Reconciler) reconcilePatroniDistributedConfiguration(
173173

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

internal/controller/postgrescluster/postgres.go

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package postgrescluster
66

77
import (
88
"bytes"
9+
"cmp"
910
"context"
1011
"fmt"
1112
"io"
@@ -29,14 +30,78 @@ import (
2930
"github.com/crunchydata/postgres-operator/internal/initialize"
3031
"github.com/crunchydata/postgres-operator/internal/logging"
3132
"github.com/crunchydata/postgres-operator/internal/naming"
33+
"github.com/crunchydata/postgres-operator/internal/patroni"
3234
"github.com/crunchydata/postgres-operator/internal/pgaudit"
35+
"github.com/crunchydata/postgres-operator/internal/pgbackrest"
36+
"github.com/crunchydata/postgres-operator/internal/pgmonitor"
3337
"github.com/crunchydata/postgres-operator/internal/postgis"
3438
"github.com/crunchydata/postgres-operator/internal/postgres"
3539
pgpassword "github.com/crunchydata/postgres-operator/internal/postgres/password"
3640
"github.com/crunchydata/postgres-operator/internal/util"
3741
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
3842
)
3943

44+
// generatePostgresParameters produces the parameter set for cluster that
45+
// incorporates, from highest to lowest precedence:
46+
// 1. mandatory values determined by controllers
47+
// 2. parameters in cluster.spec.config.parameters
48+
// 3. parameters in cluster.spec.patroni.dynamicConfiguration
49+
// 4. default values determined by contollers
50+
func (*Reconciler) generatePostgresParameters(
51+
ctx context.Context, cluster *v1beta1.PostgresCluster, backupsSpecFound bool,
52+
) *postgres.ParameterSet {
53+
builtin := postgres.NewParameters()
54+
pgaudit.PostgreSQLParameters(&builtin)
55+
pgbackrest.PostgreSQL(cluster, &builtin, backupsSpecFound)
56+
pgmonitor.PostgreSQLParameters(ctx, cluster, &builtin)
57+
postgres.SetHugePages(cluster, &builtin)
58+
59+
// Last write wins, so start with the recommended defaults.
60+
result := cmp.Or(builtin.Default.DeepCopy(), postgres.NewParameterSet())
61+
62+
// Overwrite the above with any parameters specified in the Patroni section.
63+
for k, v := range patroni.PostgresParameters(cluster.Spec.Patroni).AsMap() {
64+
result.Add(k, v)
65+
}
66+
67+
// Overwrite the above with any parameters specified in the Config section.
68+
if config := cluster.Spec.Config; config != nil {
69+
for k, v := range config.Parameters {
70+
result.Add(k, v.String())
71+
}
72+
}
73+
74+
// Overwrite the above with mandatory values.
75+
if builtin.Mandatory != nil {
76+
// This parameter is a comma-separated list. Rather than overwrite the
77+
// user-defined value, we want to combine it with the mandatory one.
78+
preload := result.Value("shared_preload_libraries")
79+
80+
for k, v := range builtin.Mandatory.AsMap() {
81+
// Load mandatory libraries ahead of user-defined libraries.
82+
if k == "shared_preload_libraries" && len(v) > 0 && len(preload) > 0 {
83+
v = v + "," + preload
84+
}
85+
86+
result.Add(k, v)
87+
}
88+
}
89+
90+
// Some preload libraries belong at specific positions in this list.
91+
if preload, ok := result.Get("shared_preload_libraries"); ok {
92+
// Load "citus" ahead of any other libraries.
93+
// - https://github.com/citusdata/citus/blob/v12.0.0/src/backend/distributed/shared_library_init.c#L417-L419
94+
// - https://github.com/citusdata/citus/blob/v13.0.0/src/backend/distributed/shared_library_init.c#L420-L422
95+
if strings.Contains(preload, "citus") {
96+
preload = "citus," + preload
97+
}
98+
99+
result.Add("shared_preload_libraries", preload)
100+
}
101+
102+
return result
103+
}
104+
40105
// generatePostgresUserSecret returns a Secret containing a password and
41106
// connection details for the first database in spec. When existing is nil or
42107
// lacks a password or verifier, a new password and verifier are generated.

internal/controller/postgrescluster/postgres_test.go

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,123 @@ import (
3434
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
3535
)
3636

37+
func TestGeneratePostgresParameters(t *testing.T) {
38+
ctx := context.Background()
39+
reconciler := &Reconciler{}
40+
41+
builtin := reconciler.generatePostgresParameters(ctx, v1beta1.NewPostgresCluster(), false)
42+
assert.Assert(t, len(builtin.AsMap()) > 0,
43+
"expected an empty cluster to have some builtin parameters")
44+
45+
assert.Equal(t, builtin.Value("jit"), "off",
46+
"BUG IN TEST: expected JIT to be disabled")
47+
48+
assert.Equal(t, builtin.Value("shared_preload_libraries"), "pgaudit",
49+
"BUG IN TEST: expected pgAudit to be mandatory")
50+
51+
t.Run("Config", func(t *testing.T) {
52+
cluster := v1beta1.NewPostgresCluster()
53+
require.UnmarshalInto(t, &cluster.Spec.Config, `{
54+
parameters: {
55+
something: str,
56+
another: 5,
57+
},
58+
}`)
59+
60+
result := reconciler.generatePostgresParameters(ctx, cluster, false)
61+
assert.Assert(t, cmp.LenMap(result.AsMap(), len(builtin.AsMap())+2),
62+
"expected two parameters from the Config section")
63+
64+
assert.Equal(t, result.Value("another"), "5")
65+
assert.Equal(t, result.Value("something"), "str")
66+
})
67+
68+
t.Run("Patroni", func(t *testing.T) {
69+
cluster := v1beta1.NewPostgresCluster()
70+
require.UnmarshalInto(t, &cluster.Spec.Patroni, `{
71+
dynamicConfiguration: {
72+
postgresql: { parameters: {
73+
something: str,
74+
another: 5.1,
75+
} },
76+
},
77+
}`)
78+
79+
result := reconciler.generatePostgresParameters(ctx, cluster, false)
80+
assert.Assert(t, cmp.LenMap(result.AsMap(), len(builtin.AsMap())+2),
81+
"expected two parameters from the Patroni section")
82+
83+
assert.Equal(t, result.Value("another"), "5.1")
84+
assert.Equal(t, result.Value("something"), "str")
85+
})
86+
87+
t.Run("Precedence", func(t *testing.T) {
88+
cluster := v1beta1.NewPostgresCluster()
89+
require.UnmarshalInto(t, &cluster.Spec.Config, `{
90+
parameters: {
91+
something: replaced,
92+
unrelated: used,
93+
jit: "on",
94+
},
95+
}`)
96+
require.UnmarshalInto(t, &cluster.Spec.Patroni, `{
97+
dynamicConfiguration: {
98+
postgresql: { parameters: {
99+
something: str,
100+
another: 5.1,
101+
} },
102+
},
103+
}`)
104+
105+
result := reconciler.generatePostgresParameters(ctx, cluster, false)
106+
assert.Assert(t, cmp.LenMap(result.AsMap(), len(builtin.AsMap())+3+1-1),
107+
"expected three parameters from the Config section,"+
108+
"plus one from the Patroni section, minus one default")
109+
110+
assert.Equal(t, result.Value("another"), "5.1") // Patroni
111+
assert.Equal(t, result.Value("something"), "replaced") // Config
112+
assert.Equal(t, result.Value("unrelated"), "used") // Config
113+
assert.Equal(t, result.Value("jit"), "on") // Config
114+
})
115+
116+
t.Run("shared_preload_libraries", func(t *testing.T) {
117+
t.Run("NumericIncluded", func(t *testing.T) {
118+
cluster := v1beta1.NewPostgresCluster()
119+
require.UnmarshalInto(t, &cluster.Spec.Config, `{
120+
parameters: {
121+
shared_preload_libraries: 123,
122+
},
123+
}`)
124+
125+
result := reconciler.generatePostgresParameters(ctx, cluster, false)
126+
assert.Assert(t, cmp.Contains(result.Value("shared_preload_libraries"), "123"))
127+
})
128+
129+
t.Run("Precedence", func(t *testing.T) {
130+
cluster := v1beta1.NewPostgresCluster()
131+
require.UnmarshalInto(t, &cluster.Spec.Config, `{
132+
parameters: {
133+
shared_preload_libraries: given,
134+
},
135+
}`)
136+
137+
result := reconciler.generatePostgresParameters(ctx, cluster, false)
138+
assert.Equal(t, result.Value("shared_preload_libraries"), "pgaudit,given",
139+
"expected mandatory ahead of specified")
140+
141+
require.UnmarshalInto(t, &cluster.Spec.Config, `{
142+
parameters: {
143+
shared_preload_libraries: 'given, citus,other'
144+
},
145+
}`)
146+
147+
result = reconciler.generatePostgresParameters(ctx, cluster, false)
148+
assert.Equal(t, result.Value("shared_preload_libraries"), "citus,pgaudit,given, citus,other",
149+
"expected citus in front")
150+
})
151+
})
152+
}
153+
37154
func TestGeneratePostgresUserSecret(t *testing.T) {
38155
_, tClient := setupKubernetes(t)
39156
require.ParallelCapacity(t, 0)

0 commit comments

Comments
 (0)