Pass Postgres settings to RestoreCommand using ParameterSet

Author: cbandy

internal/controller/postgrescluster/pgbackrest.go

@@ -1260,17 +1260,15 @@ func (r *Reconciler) reconcileRestoreJob(ctx context.Context,
 		}
 	}
 
-	// Check to see if huge pages have been requested in the spec. If they have, include 'huge_pages = try'
-	// in the restore command. If they haven't, include 'huge_pages = off'.
-	hugePagesSetting := "off"
-	if postgres.HugePagesRequested(cluster) {
-		hugePagesSetting = "try"
+	params := postgres.NewParameterSet()
+	postgres.SetHugePages(cluster, params)
+	if fetchKeyCommand := config.FetchKeyCommand(&cluster.Spec); fetchKeyCommand != "" {
+		params.Add("encryption_key_command", fetchKeyCommand)
 	}
 
 	// NOTE (andrewlecuyer): Forcing users to put each argument separately might prevent the need
 	// to do any escaping or use eval.
-	cmd := pgbackrest.RestoreCommand(pgdata, hugePagesSetting, config.FetchKeyCommand(&cluster.Spec),
-		pgtablespaceVolumes, strings.Join(opts, " "))
+	cmd := pgbackrest.RestoreCommand(pgdata, params, strings.Join(opts, " "))
 
 	// create the volume resources required for the postgres data directory
 	dataVolumeMount := postgres.DataVolumeMount()

internal/controller/postgrescluster/postgres.go

@@ -134,7 +134,7 @@ func (r *Reconciler) generatePostgresParameters(
 	pgaudit.PostgreSQLParameters(&builtin)
 	pgbackrest.PostgreSQLParameters(cluster, &builtin, backupsSpecFound)
 	pgmonitor.PostgreSQLParameters(ctx, cluster, &builtin)
-	postgres.SetHugePages(cluster, &builtin)
+	postgres.SetHugePages(cluster, builtin.Default)
 
 	// Last write wins, so start with the recommended defaults.
 	result := cmp.Or(builtin.Default.DeepCopy(), postgres.NewParameterSet())

internal/pgbackrest/config.go

@@ -212,10 +212,9 @@ func MakePGBackrestLogDir(template *corev1.PodTemplateSpec,
 //   - Renames the data directory as needed to bootstrap the cluster using the restored database.
 //     This ensures compatibility with the "existing" bootstrap method that is included in the
 //     Patroni config when bootstrapping a cluster using an existing data directory.
-func RestoreCommand(pgdata, hugePagesSetting, fetchKeyCommand string, _ []*corev1.PersistentVolumeClaim, args ...string) []string {
-	ps := postgres.NewParameterSet()
+func RestoreCommand(pgdata string, params *postgres.ParameterSet, args ...string) []string {
+	ps := params.DeepCopy()
 	ps.Add("data_directory", pgdata)
-	ps.Add("huge_pages", hugePagesSetting)
 
 	// Keep history and WAL files until the cluster starts with its normal
 	// archiving enabled.
@@ -226,10 +225,6 @@ func RestoreCommand(pgdata, hugePagesSetting, fetchKeyCommand string, _ []*corev
 	// progress during recovery.
 	ps.Add("hot_standby", "on")
 
-	if fetchKeyCommand != "" {
-		ps.Add("encryption_key_command", fetchKeyCommand)
-	}
-
 	configure := strings.Join([]string{
 		// With "hot_standby" on, some parameters cannot be smaller than they were
 		// when Postgres was backed up. Configure these to match values reported by

internal/pgbackrest/config_test.go

@@ -18,6 +18,7 @@ import (
 
 	"github.com/crunchydata/postgres-operator/internal/initialize"
 	"github.com/crunchydata/postgres-operator/internal/naming"
+	"github.com/crunchydata/postgres-operator/internal/postgres"
 	"github.com/crunchydata/postgres-operator/internal/testing/cmp"
 	"github.com/crunchydata/postgres-operator/internal/testing/require"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
@@ -789,11 +790,7 @@ func TestReloadCommandPrettyYAML(t *testing.T) {
 func TestRestoreCommand(t *testing.T) {
 	shellcheck := require.ShellCheck(t)
 
-	pgdata := "/pgdata/pg13"
-	opts := []string{
-		"--stanza=" + DefaultStanzaName, "--pg1-path=" + pgdata,
-		"--repo=1"}
-	command := RestoreCommand(pgdata, "try", "", nil, strings.Join(opts, " "))
+	command := RestoreCommand("/pgdata/pg13", postgres.NewParameterSet(), "--repo=1")
 
 	assert.DeepEqual(t, command[:3], []string{"bash", "-ceu", "--"})
 	assert.Assert(t, len(command) > 3)
@@ -810,17 +807,20 @@ func TestRestoreCommand(t *testing.T) {
 func TestRestoreCommandPrettyYAML(t *testing.T) {
 	assert.Assert(t,
 		cmp.MarshalContains(
-			RestoreCommand("/dir", "try", "", nil, "--options"),
+			RestoreCommand("/dir", postgres.NewParameterSet(), "--options"),
 			"\n- |",
 		),
 		"expected literal block scalar")
 }
 
 func TestRestoreCommandTDE(t *testing.T) {
+	params := postgres.NewParameterSet()
+	params.Add("encryption_key_command", "whatever")
+
 	assert.Assert(t,
 		cmp.MarshalContains(
-			RestoreCommand("/dir", "try", "echo testValue", nil, "--options"),
-			"encryption_key_command = 'echo testValue'",
+			RestoreCommand("/dir", params, "--options"),
+			"encryption_key_command = 'whatever'",
 		),
 		"expected encryption_key_command setting")
 }

internal/postgres/huge_pages.go

@@ -16,11 +16,11 @@ import (
 // This function looks for a valid huge_pages resource request. If it finds one,
 // it sets the PostgreSQL parameter "huge_pages" to "try". If it doesn't find
 // one, it sets "huge_pages" to "off".
-func SetHugePages(cluster *v1beta1.PostgresCluster, pgParameters *Parameters) {
+func SetHugePages(cluster *v1beta1.PostgresCluster, params *ParameterSet) {
 	if HugePagesRequested(cluster) {
-		pgParameters.Default.Add("huge_pages", "try")
+		params.Add("huge_pages", "try")
 	} else {
-		pgParameters.Default.Add("huge_pages", "off")
+		params.Add("huge_pages", "off")
 	}
 }
 

internal/postgres/huge_pages_test.go

@@ -27,11 +27,11 @@ func TestSetHugePages(t *testing.T) {
 			},
 		}}
 
-		pgParameters := NewParameters()
-		SetHugePages(cluster, &pgParameters)
+		params := NewParameterSet()
+		SetHugePages(cluster, params)
 
-		assert.Equal(t, pgParameters.Default.Has("huge_pages"), true)
-		assert.Equal(t, pgParameters.Default.Value("huge_pages"), "off")
+		assert.Equal(t, params.Has("huge_pages"), true)
+		assert.Equal(t, params.Value("huge_pages"), "off")
 	})
 
 	t.Run("hugepages quantity not set", func(t *testing.T) {
@@ -48,11 +48,11 @@ func TestSetHugePages(t *testing.T) {
 			},
 		}}
 
-		pgParameters := NewParameters()
-		SetHugePages(cluster, &pgParameters)
+		params := NewParameterSet()
+		SetHugePages(cluster, params)
 
-		assert.Equal(t, pgParameters.Default.Has("huge_pages"), true)
-		assert.Equal(t, pgParameters.Default.Value("huge_pages"), "off")
+		assert.Equal(t, params.Has("huge_pages"), true)
+		assert.Equal(t, params.Value("huge_pages"), "off")
 	})
 
 	t.Run("hugepages set to zero", func(t *testing.T) {
@@ -68,11 +68,11 @@ func TestSetHugePages(t *testing.T) {
 			},
 		}}
 
-		pgParameters := NewParameters()
-		SetHugePages(cluster, &pgParameters)
+		params := NewParameterSet()
+		SetHugePages(cluster, params)
 
-		assert.Equal(t, pgParameters.Default.Has("huge_pages"), true)
-		assert.Equal(t, pgParameters.Default.Value("huge_pages"), "off")
+		assert.Equal(t, params.Has("huge_pages"), true)
+		assert.Equal(t, params.Value("huge_pages"), "off")
 	})
 
 	t.Run("hugepages set correctly", func(t *testing.T) {
@@ -88,11 +88,11 @@ func TestSetHugePages(t *testing.T) {
 			},
 		}}
 
-		pgParameters := NewParameters()
-		SetHugePages(cluster, &pgParameters)
+		params := NewParameterSet()
+		SetHugePages(cluster, params)
 
-		assert.Equal(t, pgParameters.Default.Has("huge_pages"), true)
-		assert.Equal(t, pgParameters.Default.Value("huge_pages"), "try")
+		assert.Equal(t, params.Has("huge_pages"), true)
+		assert.Equal(t, params.Value("huge_pages"), "try")
 	})
 
 }