Look in more directories for restore binaries

internal/controller/postgrescluster/pgbackrest.go

@@ -11,6 +11,7 @@ import (
 	"path/filepath"
 	"reflect"
 	"regexp"
+	"slices"
 	"sort"
 	"strings"
 	"time"
@@ -1190,63 +1191,49 @@ func (r *Reconciler) reconcileRestoreJob(ctx context.Context,
 	pgdataVolume, pgwalVolume *corev1.PersistentVolumeClaim,
 	pgtablespaceVolumes []*corev1.PersistentVolumeClaim,
 	dataSource *v1beta1.PostgresClusterDataSource,
-	instanceName, instanceSetName, configHash, stanzaName string) error {
-
+	instanceName, instanceSetName, configHash, stanzaName string,
+) error {
+	hasFlag := make(map[string]bool)
+	matchFlag := regexp.MustCompile(`--[^ =]+`)
 	repoName := dataSource.RepoName
-	options := dataSource.Options
+
+	for _, input := range dataSource.Options {
+		for _, match := range matchFlag.FindAllString(input, -1) {
+			hasFlag[match] = true
+		}
+	}
 
 	// ensure options are properly set
 	// TODO (andrewlecuyer): move validation logic to a webhook
-	for _, opt := range options {
+	{
 		var msg string
 		switch {
-		// Since '--repo' can be set with or without an equals ('=') sign, we check for both
-		// usage patterns.
-		case strings.Contains(opt, "--repo=") || strings.Contains(opt, "--repo "):
+		case hasFlag["--repo"]:
 			msg = "Option '--repo' is not allowed: please use the 'repoName' field instead."
-		case strings.Contains(opt, "--stanza"):
-			msg = "Option '--stanza' is not allowed: the operator will automatically set this " +
-				"option"
-		case strings.Contains(opt, "--pg1-path"):
-			msg = "Option '--pg1-path' is not allowed: the operator will automatically set this " +
-				"option"
-		case strings.Contains(opt, "--target-action"):
-			msg = "Option '--target-action' is not allowed: the operator will automatically set this " +
-				"option "
-		case strings.Contains(opt, "--link-map"):
-			msg = "Option '--link-map' is not allowed: the operator will automatically set this " +
-				"option "
+		case hasFlag["--stanza"]:
+			msg = "Option '--stanza' is not allowed: the operator will automatically set this option"
+		case hasFlag["--pg1-path"]:
+			msg = "Option '--pg1-path' is not allowed: the operator will automatically set this option"
+		case hasFlag["--target-action"]:
+			msg = "Option '--target-action' is not allowed: the operator will automatically set this option"
+		case hasFlag["--link-map"]:
+			msg = "Option '--link-map' is not allowed: the operator will automatically set this option"
 		}
 		if msg != "" {
-			r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "InvalidDataSource", msg, repoName)
+			r.Recorder.Event(cluster, corev1.EventTypeWarning, "InvalidDataSource", msg)
 			return nil
 		}
 	}
 
 	pgdata := postgres.DataDirectory(cluster)
 	// combine options provided by user in the spec with those populated by the operator for a
 	// successful restore
-	opts := append(options, []string{
-		"--stanza=" + stanzaName,
-		"--pg1-path=" + pgdata,
-		"--repo=" + regexRepoIndex.FindString(repoName)}...)
-
-	// Look specifically for the "--target" flag, NOT flags that contain
-	// "--target" (e.g. "--target-timeline")
-	targetRegex, err := regexp.Compile("--target[ =]")
-	if err != nil {
-		return err
-	}
-	var deltaOptFound, foundTarget bool
-	for _, opt := range opts {
-		switch {
-		case targetRegex.MatchString(opt):
-			foundTarget = true
-		case strings.Contains(opt, "--delta"):
-			deltaOptFound = true
-		}
-	}
-	if !deltaOptFound {
+	opts := append(slices.Clone(dataSource.Options), shell.QuoteWords(
+		"--stanza="+stanzaName,
+		"--pg1-path="+pgdata,
+		"--repo="+regexRepoIndex.FindString(repoName),
+	)...)
+	if !hasFlag["--delta"] {
 		opts = append(opts, "--delta")
 	}
 
@@ -1262,28 +1249,26 @@ func (r *Reconciler) reconcileRestoreJob(ctx context.Context,
 	// - https://github.com/pgbackrest/pgbackrest/blob/bb03b3f41942d0b781931092a76877ad309001ef/src/command/restore/restore.c#L1623
 	// - https://github.com/pgbackrest/pgbackrest/issues/1314
 	// - https://github.com/pgbackrest/pgbackrest/issues/987
-	if foundTarget {
+	if hasFlag["--target"] {
 		opts = append(opts, "--target-action=promote")
 	}
 
 	for i, instanceSpec := range cluster.Spec.InstanceSets {
 		if instanceSpec.Name == instanceSetName {
-			opts = append(opts, "--link-map=pg_wal="+postgres.WALDirectory(cluster,
-				&cluster.Spec.InstanceSets[i]))
+			opts = append(opts, "--link-map=pg_wal="+
+				postgres.WALDirectory(cluster, &cluster.Spec.InstanceSets[i]))
 		}
 	}
 
-	// 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(cluster.Spec.PostgresVersion, pgdata, params, strings.Join(opts, " "))
 
 	// create the volume resources required for the postgres data directory
 	dataVolumeMount := postgres.DataVolumeMount()

internal/controller/postgrescluster/pgbackrest_test.go

@@ -34,6 +34,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+	"sigs.k8s.io/yaml"
 
 	"github.com/crunchydata/postgres-operator/internal/controller/runtime"
 	"github.com/crunchydata/postgres-operator/internal/initialize"
@@ -2355,7 +2356,8 @@ func TestReconcileCloudBasedDataSource(t *testing.T) {
 					LabelSelector: naming.PGBackRestRestoreJobSelector(clusterName),
 					Namespace:     cluster.Namespace,
 				}))
-				assert.Assert(t, tc.result.jobCount == len(restoreJobs.Items))
+				assert.Equal(t, tc.result.jobCount, len(restoreJobs.Items),
+					"got:\n%s", require.Value(yaml.Marshal(restoreJobs.Items)))
 				if len(restoreJobs.Items) == 1 {
 					assert.Assert(t, restoreJobs.Items[0].Labels[naming.LabelStartupInstance] != "")
 					assert.Assert(t, restoreJobs.Items[0].Annotations[naming.PGBackRestConfigHash] != "")

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(postgresVersion int32, 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
@@ -271,6 +266,7 @@ func RestoreCommand(pgdata, hugePagesSetting, fetchKeyCommand string, _ []*corev
 
 	script := strings.Join([]string{
 		`declare -r PGDATA="$1" opts="$2"; export PGDATA PGHOST`,
+		postgres.ShellPath(postgresVersion),
 
 		// Remove any "postmaster.pid" file leftover from a prior failure.
 		`rm -f "${PGDATA}/postmaster.pid"`,

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,15 +790,14 @@ 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(19, "/pgdata/pg13", postgres.NewParameterSet(), "--repo=1")
 
 	assert.DeepEqual(t, command[:3], []string{"bash", "-ceu", "--"})
 	assert.Assert(t, len(command) > 3)
 
+	assert.Assert(t, cmp.Contains(command[3], "/usr/pgsql-19/bin"),
+		"expected path to PostgreSQL binaries")
+
 	dir := t.TempDir()
 	file := filepath.Join(dir, "script.bash")
 	assert.NilError(t, os.WriteFile(file, []byte(command[3]), 0o600))
@@ -810,17 +810,20 @@ func TestRestoreCommand(t *testing.T) {
 func TestRestoreCommandPrettyYAML(t *testing.T) {
 	assert.Assert(t,
 		cmp.MarshalContains(
-			RestoreCommand("/dir", "try", "", nil, "--options"),
+			RestoreCommand(9, "/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(20, "/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")
 	})
 
 }