From 79302884b7c77153b62fd2aea0ed63c97f980416 Mon Sep 17 00:00:00 2001 From: GuptaManan100 Date: Thu, 10 Sep 2026 13:11:38 +0530 Subject: [PATCH 1/3] fix(postgresconfig): render baseline after ref so operator sizing wins Render() appended layers baseline -> ref -> inline, so the deprecated PostgresConfigRef overrode the operator's resource-derived baseline (PostgreSQL applies later assignments last-write-wins). The ref is opaque raw text: if it contains an `include` directive, PostgreSQL transitively pulls in an external file the operator never sees, silently overriding carefully sized values (shared_buffers, effective_cache_size, max_slot_wal_keep_size, ...) with whatever is baked into that file. Render the baseline AFTER the ref (ref -> baseline -> inline) so the operator's own sizing math always wins over the deprecated ref, while inline spec.postgresConfig remains the single explicit override. This inverts the documented baseline-vs-ref precedence; ref-vs-inline is unchanged (inline still last). Add a unit repro (TestBaselineWinsOverRefForResourceDerivedKeys) and update the two tests that pinned the old ordering (render_test.go, postgres_config_test.go). Signed-off-by: GuptaManan100 --- pkg/postgresconfig/render.go | 38 +++++++++------ pkg/postgresconfig/render_precedence_test.go | 46 +++++++++++++++++++ pkg/postgresconfig/render_test.go | 11 +++-- .../controller/shard/postgres_config_test.go | 11 +++-- 4 files changed, 83 insertions(+), 23 deletions(-) create mode 100644 pkg/postgresconfig/render_precedence_test.go diff --git a/pkg/postgresconfig/render.go b/pkg/postgresconfig/render.go index 1340205c..e4c40167 100644 --- a/pkg/postgresconfig/render.go +++ b/pkg/postgresconfig/render.go @@ -1,9 +1,13 @@ // Package postgresconfig renders the effective postgresql.conf the operator -// mounts into pgctld. The operator owns config generation end-to-end: a static -// baseline it defines, followed by the user's legacy PostgresConfigRef content -// and the inline spec.postgresConfig map, each appended so it overrides earlier -// layers (PostgreSQL applies later assignments last-write-wins). Resource- -// derived sizing is baked into the Config before rendering. +// mounts into pgctld. The operator owns config generation end-to-end. Layers are +// appended in order of increasing precedence (PostgreSQL applies later +// assignments last-write-wins): first the user's legacy PostgresConfigRef content +// (deprecated), then the operator's static, resource-derived baseline, then the +// inline spec.postgresConfig map. The baseline is rendered AFTER the ref on +// purpose: the ref is opaque raw text that may transitively `include` an external +// file the operator never sees, so it must never override the operator's own +// sizing math — only inline spec.postgresConfig may deviate from the baseline. +// Resource-derived sizing is baked into the Config before rendering. package postgresconfig import ( @@ -90,21 +94,25 @@ func Defaults() Config { } } -// Render produces the effective postgresql.conf: the baseline template rendered -// with cfg, followed by the user's legacy PostgresConfigRef content (verbatim) -// and the inline spec.postgresConfig map, each appended so it overrides earlier -// layers. refContent is the body of the user's PostgresConfigRef key, or empty -// when no ref is set; inline may be nil. +// Render produces the effective postgresql.conf. Layers are appended in order of +// increasing precedence (last-write-wins): the user's legacy PostgresConfigRef +// content (verbatim), then the baseline template rendered with cfg, then the +// inline spec.postgresConfig map. The baseline is rendered after the ref so the +// operator's resource-derived values always win over the deprecated ref; only +// inline overrides the baseline. refContent is the body of the user's +// PostgresConfigRef key, or empty when no ref is set; inline may be nil. func Render(cfg Config, refContent string, inline map[string]string) (string, error) { var b strings.Builder - if err := parsedBaseTemplate.Execute(&b, cfg); err != nil { - return "", fmt.Errorf("rendering postgres config template: %w", err) - } + // The deprecated ref is rendered first so the baseline (next) overrides it. if trimmed := strings.TrimRight(refContent, "\n"); trimmed != "" { - b.WriteString("\n# postgresConfigRef\n") + b.WriteString("# postgresConfigRef\n") b.WriteString(trimmed) - b.WriteString("\n") + b.WriteString("\n\n") + } + + if err := parsedBaseTemplate.Execute(&b, cfg); err != nil { + return "", fmt.Errorf("rendering postgres config template: %w", err) } if len(inline) > 0 { diff --git a/pkg/postgresconfig/render_precedence_test.go b/pkg/postgresconfig/render_precedence_test.go new file mode 100644 index 00000000..2ee1ce57 --- /dev/null +++ b/pkg/postgresconfig/render_precedence_test.go @@ -0,0 +1,46 @@ +package postgresconfig + +import "testing" + +// TestBaselineWinsOverRefForResourceDerivedKeys reproduces a precedence problem +// in Render(): the operator's own resource-derived baseline must not be +// overridable by the deprecated PostgresConfigRef. +// +// Render() appends layers in the order baseline -> ref -> inline, and PostgreSQL +// applies later assignments last-write-wins, so today ref overrides the baseline. +// That is fine when ref is a handful of direct GUC assignments, but ref is opaque +// raw text: if it contains an `include` directive, PostgreSQL transitively pulls +// in an external file the operator never sees, silently overriding the operator's +// carefully sized values (shared_buffers, effective_cache_size, ...) with +// whatever is baked into that file. Only inline spec.postgresConfig should be +// able to override the baseline; ref (deprecated) should not. +// +// This is a unit-level repro against Render()+StampAndSplit() alone — no include, +// no image, no k8s needed, because the defect lives entirely in the string +// ordering inside Render(). +// +// Today this FAILS: split.ReloadSettings["effective_cache_size"] == "999MB" (ref +// wins). After swapping the order to ref -> baseline -> inline it must be "192MB" +// (baseline wins), since no inline override was given. +func TestBaselineWinsOverRefForResourceDerivedKeys(t *testing.T) { + cfg := Defaults() // effective_cache_size baseline default is "192MB" + + // A ref value clearly different from the baseline so a precedence bug is + // unmistakable. A direct assignment stands in for whatever an opaque ref + // (or a file it includes) would set — Render() does not care which. + const refContent = "effective_cache_size = '999MB'" + + rendered, err := Render(cfg, refContent, nil) + if err != nil { + t.Fatalf("Render: %v", err) + } + + _, split := StampAndSplit(rendered) + got := split.ReloadSettings["effective_cache_size"] + if want := "192MB"; got != want { + t.Errorf( + "effective_cache_size = %q, want %q: the operator's resource-derived baseline must win over the deprecated PostgresConfigRef (only inline spec.postgresConfig should override it)", + got, want, + ) + } +} diff --git a/pkg/postgresconfig/render_test.go b/pkg/postgresconfig/render_test.go index 9b27f57e..50118645 100644 --- a/pkg/postgresconfig/render_test.go +++ b/pkg/postgresconfig/render_test.go @@ -41,7 +41,7 @@ func TestRender(t *testing.T) { } }) - t.Run("ref content appended verbatim after the baseline", func(t *testing.T) { + t.Run("ref content emitted verbatim before the baseline", func(t *testing.T) { ref := "shared_buffers = '8GB'\n# a comment" got, err := Render(Defaults(), ref, nil) if err != nil { @@ -50,9 +50,12 @@ func TestRender(t *testing.T) { if !strings.Contains(got, ref) { t.Errorf("ref content not emitted verbatim, got:\n%s", got) } - // Ref must come after the baseline so it wins last-write-wins. - if strings.Index(got, ref) < strings.Index(got, "shared_buffers = 64MB") { - t.Errorf("ref content should follow the baseline, got:\n%s", got) + // Ref must come BEFORE the baseline so the operator's resource-derived + // baseline wins last-write-wins: here the baseline's shared_buffers = 64MB + // must override the ref's 8GB. The deprecated ref may not override the + // operator's sizing math — only inline spec.postgresConfig can. + if strings.Index(got, ref) > strings.Index(got, "shared_buffers = 64MB") { + t.Errorf("ref content should precede the baseline, got:\n%s", got) } }) diff --git a/pkg/resource-handler/controller/shard/postgres_config_test.go b/pkg/resource-handler/controller/shard/postgres_config_test.go index b2ef9aaf..d67abae6 100644 --- a/pkg/resource-handler/controller/shard/postgres_config_test.go +++ b/pkg/resource-handler/controller/shard/postgres_config_test.go @@ -408,21 +408,24 @@ func TestReconcilePostgresConfig_MergesRefContent(t *testing.T) { t.Fatalf("operator ConfigMap not created: %v", err) } rendered := got.Data[PostgresConfigMapKey] - // Baseline present, and the ref override appended after it. + // Baseline present, and the ref rendered BEFORE it so the operator's + // resource-derived baseline wins last-write-wins: the baseline's + // shared_buffers = 64MB must override the ref's 8GB. The deprecated ref must + // not override the operator's sizing math — only inline spec.postgresConfig. if !strings.Contains(rendered, "shared_buffers = 64MB") { t.Errorf("rendered config missing baseline:\n%s", rendered) } if !strings.Contains(rendered, "shared_buffers = '8GB'") { - t.Errorf("rendered config missing ref override:\n%s", rendered) + t.Errorf("rendered config missing ref content:\n%s", rendered) } if strings.Index( rendered, "shared_buffers = '8GB'", - ) < strings.Index( + ) > strings.Index( rendered, "shared_buffers = 64MB", ) { - t.Errorf("ref override should follow the baseline:\n%s", rendered) + t.Errorf("ref content should precede the baseline so the baseline wins:\n%s", rendered) } } From 8d1ab14925dc80e4eba390f0a973f9a17e319754 Mon Sep 17 00:00:00 2001 From: GuptaManan100 Date: Thu, 10 Sep 2026 19:19:18 +0530 Subject: [PATCH 2/3] test(e2e): assert operator baseline wins over PostgresConfigRef Add TestBaselineWinsOverRef: create a shard with a postgresConfigRef that sets a resource-derived key (effective_cache_size) to a value the operator's sizing would never produce, and no inline override. It asserts the effective value is the operator's sized baseline (384MB at a 512Mi pool limit), not the ref's 999MB. This fails against the pre-fix operator (ref wins) and passes after (baseline wins). It also asserts a genuinely baseline-absent ref-only key (seq_page_cost) still applies, proving the ref is layered under the baseline rather than discarded. Fix the existing "legacy postgresConfigRef is still honored" subtest, which used random_page_cost as its ref-only probe. random_page_cost is set by the operator baseline (1.1), so with the baseline now winning over the ref the old assertion (2.5) no longer holds; switch it to seq_page_cost, which the baseline does not set. Signed-off-by: GuptaManan100 --- .../postgresconfig/baseline_over_ref_test.go | 92 +++++++++++++++++++ .../postgresconfig/postgresconfig_test.go | 16 ++-- 2 files changed, 102 insertions(+), 6 deletions(-) create mode 100644 test/e2e/shared/postgresconfig/baseline_over_ref_test.go diff --git a/test/e2e/shared/postgresconfig/baseline_over_ref_test.go b/test/e2e/shared/postgresconfig/baseline_over_ref_test.go new file mode 100644 index 00000000..5115dfe1 --- /dev/null +++ b/test/e2e/shared/postgresconfig/baseline_over_ref_test.go @@ -0,0 +1,92 @@ +//go:build e2e + +package postgresconfig_test + +import ( + "context" + "testing" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + multigresv1alpha1 "github.com/multigres/multigres-operator/api/v1alpha1" + "github.com/multigres/multigres-operator/test/e2e/framework" +) + +// TestBaselineWinsOverRef verifies end-to-end that the operator's own +// resource-derived baseline wins over the deprecated PostgresConfigRef. +// +// This is the real-project shape that failed: a shard is created with a +// postgresConfigRef still present (the worker's create-time wiring) that sets a +// resource-derived key — here effective_cache_size — to a value the operator's +// sizing would never produce, and NO inline override. With a 512Mi pool memory +// limit the operator sizes effective_cache_size to mem*3/4 = 384MB; the ref sets +// 999MB. +// +// - Before the fix (baseline rendered BEFORE ref): ref wins last-write-wins, so +// SHOW effective_cache_size == "999MB" and this test FAILS. +// - After the fix (baseline rendered AFTER ref): the operator's 384MB wins and +// this test PASSES. +// +// It also asserts a ref-only key (seq_page_cost, which the baseline does not set) +// still applies, proving the fix layers the ref UNDER the baseline rather than +// ignoring it. +func TestBaselineWinsOverRef(t *testing.T) { + ns := cluster.CreateNamespace(t) + c, err := cluster.CRClient() + if err != nil { + t.Fatalf("create CR client: %v", err) + } + ctx := context.Background() + + // Deprecated ref (key "postgresql.conf", like the real project): sets a + // resource-derived baseline key to a value the operator would never size to, + // plus a ref-only key that the baseline does not set. + refCM := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "pg-ref", Namespace: ns}, + Data: map[string]string{ + // effective_cache_size is set by the operator's baseline (the contested + // key); seq_page_cost is NOT set by the baseline (a genuinely ref-only + // key, to prove the ref is still layered in, not discarded). + "postgresql.conf": "effective_cache_size = '999MB'\nseq_page_cost = '2.5'", + }, + } + if err := c.Create(ctx, refCM); err != nil { + t.Fatalf("create ref ConfigMap: %v", err) + } + + // Create with the ref set and NO inline postgresConfig, and pin the pool + // memory to 512Mi so the operator's sized effective_cache_size is a + // deterministic 384MB (= mem*3/4). + cr := framework.MustLoadCluster("config/samples/no-templates.yaml", ns) + framework.WithCIResources(&cr.Spec) + shard := &cr.Spec.Databases[0].TableGroups[0].Shards[0] + shard.Spec.PostgresConfigRef = &multigresv1alpha1.PostgresConfigRef{ + Name: "pg-ref", + Key: "postgresql.conf", + } + for name, pool := range shard.Spec.Pools { + if pool.Postgres.Resources.Limits == nil { + pool.Postgres.Resources.Limits = corev1.ResourceList{} + } + pool.Postgres.Resources.Limits[corev1.ResourceMemory] = resource.MustParse("512Mi") + shard.Spec.Pools[name] = pool + } + if err := c.Create(ctx, cr); err != nil { + t.Fatalf("create MultigresCluster: %v", err) + } + + framework.WaitForPod(t, c, ns, "postgres") + cluster.WaitForAllPodsReady(t, ns) + gw := framework.FindGatewayService(t, cluster, ns) + framework.WaitForQueryServing(t, cluster, ns, gw) + + // The operator's resource-derived baseline must win over the ref: 384MB, not + // the ref's 999MB. This is the assertion that fails before the fix. + framework.WaitForPsqlValue(t, cluster, ns, gw, "SHOW effective_cache_size", "384MB") + + // The ref is still honored for keys the baseline does not set — it is layered + // UNDER the baseline, not discarded. + framework.WaitForPsqlValue(t, cluster, ns, gw, "SHOW seq_page_cost", "2.5") +} diff --git a/test/e2e/shared/postgresconfig/postgresconfig_test.go b/test/e2e/shared/postgresconfig/postgresconfig_test.go index 656fcbed..53dd8d72 100644 --- a/test/e2e/shared/postgresconfig/postgresconfig_test.go +++ b/test/e2e/shared/postgresconfig/postgresconfig_test.go @@ -33,13 +33,14 @@ func TestPostgresConfigManagement(t *testing.T) { } ctx := context.Background() - // Legacy postgresConfigRef ConfigMap: sets random_page_cost (not in the - // inline map, to prove the ref is honored) and work_mem (which the inline map - // overrides, to prove precedence). + // Legacy postgresConfigRef ConfigMap: sets seq_page_cost (a key the operator's + // baseline does NOT set and that is not in the inline map, to prove the ref is + // honored for keys the baseline leaves alone) and work_mem (which the inline + // map overrides, to prove precedence). refCM := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: "pg-ref", Namespace: ns}, Data: map[string]string{ - "custom.conf": "random_page_cost = '2.5'\nwork_mem = '64MB'", + "custom.conf": "seq_page_cost = '2.5'\nwork_mem = '64MB'", }, } if err := c.Create(ctx, refCM); err != nil { @@ -78,8 +79,11 @@ func TestPostgresConfigManagement(t *testing.T) { }) t.Run("legacy postgresConfigRef is still honored", func(t *testing.T) { - // random_page_cost is only in the ref (not the map), so it must apply. - framework.WaitForPsqlValue(t, cluster, ns, gw, "SHOW random_page_cost", "2.5") + // seq_page_cost is set only by the ref — not by the operator baseline and + // not by the inline map — so the ref value must apply. (The baseline is now + // rendered after the ref, so this proves the ref is still layered in for + // keys the baseline does not set, rather than being discarded.) + framework.WaitForPsqlValue(t, cluster, ns, gw, "SHOW seq_page_cost", "2.5") }) t.Run("operator renders a per-shard ConfigMap", func(t *testing.T) { From 72c5ff798358f96dd82b532498c72a72a7af39c5 Mon Sep 17 00:00:00 2001 From: GuptaManan100 Date: Thu, 10 Sep 2026 20:42:52 +0530 Subject: [PATCH 3/3] feat: fix lint issues Signed-off-by: GuptaManan100 --- pkg/postgresconfig/render_precedence_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/postgresconfig/render_precedence_test.go b/pkg/postgresconfig/render_precedence_test.go index 2ee1ce57..e2ade030 100644 --- a/pkg/postgresconfig/render_precedence_test.go +++ b/pkg/postgresconfig/render_precedence_test.go @@ -40,7 +40,8 @@ func TestBaselineWinsOverRefForResourceDerivedKeys(t *testing.T) { if want := "192MB"; got != want { t.Errorf( "effective_cache_size = %q, want %q: the operator's resource-derived baseline must win over the deprecated PostgresConfigRef (only inline spec.postgresConfig should override it)", - got, want, + got, + want, ) } }