fix(postgresconfig): render baseline after ref so operator sizing wins

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 {

pkg/postgresconfig/render_precedence_test.go

@@ -0,0 +1,47 @@
+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,
+		)
+	}
+}

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)
 		}
 	})
 

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)
 	}
 }
 

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")
+}

test/e2e/shared/postgresconfig/postgresconfig_test.go

@@ -32,13 +32,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 {
@@ -77,8 +78,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) {