Make the default Postgres log directory outside its data directory

This has been the recommended practice for ages, but PGO left this
parameter at Postgres' packaged default. Being outside the data
directory makes the log directory and files:

 - easier to create
 - easier to consume from the OpenTelemetry Collector
 - easier to exclude from backups
 - persistent across failures that recover by recreating the data directory

Users that wish to log to the default directory can
set "spec.config.parameters.log_directory" to "log" on PostgresCluster.

Co-authored-by: Benjamin Blattberg <ben.blattberg@crunchydata.com>
Co-authored-by: Drew Sessler <drew.sessler@crunchydata.com>
Issue: PGO-2558

Author: 3 people

internal/collector/instance.go

@@ -181,11 +181,8 @@ func AddToPod(
 // startCommand generates the command script used by the collector container
 func startCommand(logDirectories []string, includeLogrotate bool) []string {
 	var mkdirScript string
-	if len(logDirectories) != 0 {
-		for _, logDir := range logDirectories {
-			mkdirScript = mkdirScript + `
-` + shell.MakeDirectories(logDir, "receiver")
-		}
+	for _, logDir := range logDirectories {
+		mkdirScript += "\n" + shell.MakeDirectories(logDir, "receiver")
 	}
 
 	var logrotateCommand string

internal/collector/postgres.go

@@ -64,9 +64,6 @@ func PostgreSQLParameters(ctx context.Context,
 
 		// Log in a timezone the OpenTelemetry Collector understands.
 		outParameters.Mandatory.Add("log_timezone", "UTC")
-
-		// TODO(logs): Remove this call and do it in [postgres.NewParameters] regardless of the gate.
-		outParameters.Mandatory.Add("log_directory", fmt.Sprintf("%s/logs/postgres", postgres.DataStorage(inCluster)))
 	}
 }
 

internal/collector/postgres_test.go

@@ -33,9 +33,7 @@ func TestEnablePostgresLogging(t *testing.T) {
 		config := NewConfig(nil)
 		params := postgres.NewParameters()
 
-		// NOTE: This call is necessary only because the default "log_directory" is not absolute.
-		PostgreSQLParameters(ctx, cluster, &params)
-		EnablePostgresLogging(ctx, cluster, params.Mandatory, config)
+		EnablePostgresLogging(ctx, cluster, params.Default, config)
 
 		result, err := config.ToYAML()
 		assert.NilError(t, err)
@@ -297,9 +295,7 @@ service:
 		config := NewConfig(cluster.Spec.Instrumentation)
 		params := postgres.NewParameters()
 
-		// NOTE: This call is necessary only because the default "log_directory" is not absolute.
-		PostgreSQLParameters(ctx, cluster, &params)
-		EnablePostgresLogging(ctx, cluster, params.Mandatory, config)
+		EnablePostgresLogging(ctx, cluster, params.Default, config)
 
 		result, err := config.ToYAML()
 		assert.NilError(t, err)

internal/controller/postgrescluster/instance.go

@@ -1194,6 +1194,10 @@ func (r *Reconciler) reconcileInstance(
 		// set includeLogrotate to true, but only if backups are enabled
 		// and local volumes are available.
 		includeLogrotate := backupsSpecFound && pgbackrest.RepoHostVolumeDefined(cluster)
+
+		// The log directories here do not include Postgres "log_directory" because it might be a subdirectory of "data_directory".
+		// In that case, the "log_directory" must be created *after* initdb runs, not during container startup here.
+		// TODO(sidecar): Create these directories sometime other than startup.
 		collector.AddToPod(ctx, cluster.Spec.Instrumentation, cluster.Spec.ImagePullPolicy, instanceConfigMap, &instance.Spec.Template,
 			[]corev1.VolumeMount{postgres.DataVolumeMount()}, pgPassword,
 			[]string{naming.PGBackRestPGDataLogPath}, includeLogrotate, true)

internal/postgres/parameters.go

@@ -7,6 +7,7 @@ package postgres
 import (
 	"fmt"
 	"maps"
+	"path"
 	"slices"
 	"strings"
 )
@@ -48,16 +49,13 @@ func NewParameters() Parameters {
 	// - https://www.postgresql.org/docs/current/auth-password.html
 	parameters.Default.Add("password_encryption", "scram-sha-256")
 
-	// Explicitly use Postgres' default log directory. This value is relative to the "data_directory" parameter.
+	// Log outside of the Postgres data directory by default.
 	//
-	// Controllers look at this parameter to grant group-write S_IWGRP on the directory.
-	// Postgres does not grant this permission on the directories it creates.
-	//
-	// TODO(logs): A better default would be *outside* the data directory.
-	// This parameter needs to be configurable and documented before the default can change.
+	// When log files are inside the data directory, they are destroyed along with the data directory
+	// during replica creation and major upgrades. Being outside also reduces the size of backups.
 	//
 	// PostgreSQL must be reloaded when changing this parameter.
-	parameters.Default.Add("log_directory", "log")
+	parameters.Default.Add("log_directory", path.Join(dataMountPath, "logs/postgres"))
 
 	// Pod "securityContext.fsGroup" ensures processes and filesystems agree on a GID;
 	// use the same permissions for group and owner.

internal/postgres/parameters_test.go

@@ -28,8 +28,7 @@ func TestNewParameters(t *testing.T) {
 	assert.DeepEqual(t, parameters.Default.AsMap(), map[string]string{
 		"jit": "off",
 
-		"log_directory": "log",
-
+		"log_directory":       "/pgdata/logs/postgres",
 		"password_encryption": "scram-sha-256",
 	})
 }

internal/postgres/reconcile_test.go

@@ -288,9 +288,8 @@ initContainers:
     bootstrap_dir="${postgres_data_directory}_bootstrap"
     [[ -d "${bootstrap_dir}" ]] && postgres_data_directory="${bootstrap_dir}" && results 'bootstrap directory' "${bootstrap_dir}"
     dataDirectory "${postgres_data_directory}" || halt "$(permissions "${postgres_data_directory}" ||:)"
-    [[ ! -f '/pgdata/pg11/PG_VERSION' ]] ||
-    (mkdir -p '/pgdata/pg11/log' && { chmod 0775 '/pgdata/pg11/log' || :; }) ||
-    halt "$(permissions '/pgdata/pg11/log' ||:)"
+    (mkdir -p '/pgdata/logs/postgres' && { chmod 0775 '/pgdata/logs/postgres' '/pgdata/logs' || :; }) ||
+    halt "$(permissions '/pgdata/logs/postgres' ||:)"
     (mkdir -p '/pgdata/patroni/log' && { chmod 0775 '/pgdata/patroni/log' '/pgdata/patroni' || :; }) ||
     halt "$(permissions '/pgdata/patroni/log' ||:)"
     (mkdir -p '/pgdata/pgbackrest/log' && { chmod 0775 '/pgdata/pgbackrest/log' '/pgdata/pgbackrest' || :; }) ||