Repository: cloudnative-pg/cloudnative-pg

Issue:
[Bug]: Subscription update with copy_data parameter
### Is there an existing issue already for this bug?

- [x] I have searched for an existing issue, and could not find anything. I believe this is a new bug.

### I have read the troubleshooting guide

- [x] I have read the troubleshooting guide and I think this is a new bug.

### I am running a supported version of CloudNativePG

- [x] I have read the troubleshooting guide and I think this is a new bug.

### Contact Details

thomas.loubiou@mirakl.com

### Version

1.25 (latest patch)

### What version of Kubernetes are you using?

1.32

### What is your Kubernetes environment?

Cloud: Amazon EKS

### How did you install the operator?

Helm

### What happened?

We are creating some subscriptions like the following:

```yaml
apiVersion: postgresql.cnpg.io/v1
kind: Subscription
metadata:
  name: my_sub
spec:
  cluster:
    name: my_cluster
  dbname: my_db
  externalClusterName: internal-pg12
  name: pg12_pg17
  parameters:
    copy_data: "true"
  publicationName: inter_cluster
  subscriptionReclaimPolicy: delete
```

The creation of the subscription works great, but in case of an update the reconciliation fails because of the `copy_data` field.

After investigating I've found that the [update code](https://github.com/cloudnative-pg/cloudnative-pg/blob/874528a4c98bad0eedfc80a634f4949c4330e5a6/internal/management/controller/subscription_controller_sql.go#L131)  generate a naive `ALTER SUBSCRIPTION xxx SET copy_data = true`, which is an [invalid postgreSQL statement](https://www.postgresql.org/docs/17/sql-altersubscription.html#SQL-ALTERSUBSCRIPTION-PARAMS-SET). Indeed `ALTER SUBSCRIPTION` cannot change the `copy_data` parameter, from the official doc:

> The parameters that can be altered are [slot_name](https://www.postgresql.org/docs/17/sql-createsubscription.html#SQL-CREATESUBSCRIPTION-PARAMS-WITH-SLOT-NAME), [synchronous_commit](https://www.postgresql.org/docs/17/sql-createsubscription.html#SQL-CREATESUBSCRIPTION-PARAMS-WITH-SYNCHRONOUS-COMMIT), [binary](https://www.postgresql.org/docs/17/sql-createsubscription.html#SQL-CREATESUBSCRIPTION-PARAMS-WITH-BINARY), [streaming](https://www.postgresql.org/docs/17/sql-createsubscription.html#SQL-CREATESUBSCRIPTION-PARAMS-WITH-STREAMING), [disable_on_error](https://www.postgresql.org/docs/17/sql-createsubscription.html#SQL-CREATESUBSCRIPTION-PARAMS-WITH-DISABLE-ON-ERROR), [password_required](https://www.postgresql.org/docs/17/sql-createsubscription.html#SQL-CREATESUBSCRIPTION-PARAMS-WITH-PASSWORD-REQUIRED), [run_as_owner](https://www.postgresql.org/docs/17/sql-createsubscription.html#SQL-CREATESUBSCRIPTION-PARAMS-WITH-RUN-AS-OWNER), [origin](https://www.postgresql.org/docs/17/sql-createsubscription.html#SQL-CREATESUBSCRIPTION-PARAMS-WITH-ORIGIN), and [failover](https://www.postgresql.org/docs/17/sql-createsubscription.html#SQL-CREATESUBSCRIPTION-PARAMS-WITH-FAILOVER). Only a superuser can set password_required = false.

Please note that we don't change this `copy_data` parameter.

The subscription seems to be created by the 1st reconciliation, but after it is impossible to update it using the operator.

Here is the observed status:

```yaml
status:
  applied: false
  message: 'while patching subscription: ERROR: unrecognized subscription parameter:
    "copy_data" (SQLSTATE 42601)'
  observedGeneration: 1
```

### Code of Conduct

- [x] I agree to follow this project's Code of Conduct

Discussion:
@dosubot: <!-- Greeting -->
Hello @xabufr! I'm here to help you with bugs, answer your questions, and support you as you become a contributor. Feel free to ask anything while we wait for a human maintainer to assist you!

<!-- Answer -->
The issue you're encountering is due to the `copy_data` parameter being included in the update logic for subscriptions, which is not supported by PostgreSQL's `ALTER SUBSCRIPTION` command. The `copy_data` parameter can only be used during the creation of a subscription and not during updates.

To address this issue, you should ensure that the `copy_data` parameter is not included in the update logic of your subscription management. Since the `copy_data` parameter is not meant to be altered after the initial creation, you should remove it from any update operations.

If the operator is automatically including `copy_data` in updates, this might be a bug in the operator's logic. You can consider modifying the operator's code to exclude `copy_data` from the update SQL generation or wait for a fix if this is a known issue.

For now, as a workaround, you can manually manage the subscription updates without altering the `copy_data` parameter, or adjust the operator's logic to prevent it from attempting to update this parameter.


<!-- Dosu Comment Footer -->
*To continue talking to [Dosu](https://dosu.dev), mention @dosu.*

---
<sup>Help Dosu learn! Give it feedback: [Great Response](https://app.dosu.dev/response-feedback/78cf6537-6dd0-4c4c-9908-837cddfd09d6?feedback_type=great_response) | [Irrelevant Answer](https://app.dosu.dev/response-feedback/78cf6537-6dd0-4c4c-9908-837cddfd09d6?feedback_type=irrelevant_answer) | [Incorrect Sources](https://app.dosu.dev/response-feedback/78cf6537-6dd0-4c4c-9908-837cddfd09d6?feedback_type=incorrect_sources) | [Too Verbose](https://app.dosu.dev/response-feedback/78cf6537-6dd0-4c4c-9908-837cddfd09d6?feedback_type=too_verbose) | [Hallucination](https://app.dosu.dev/response-feedback/78cf6537-6dd0-4c4c-9908-837cddfd09d6?feedback_type=hallucination) | [Bug Report](https://app.dosu.dev/response-feedback/78cf6537-6dd0-4c4c-9908-837cddfd09d6?feedback_type=bug_report) | [Other](https://app.dosu.dev/response-feedback/78cf6537-6dd0-4c4c-9908-837cddfd09d6?feedback_type=other)</sup>
@xabufr: It seems I also randomly encounter this bug on subscriptions I never updated, on creation. After investigating I think sometimes the cache is not updated faster enough and old resource is returned.

Here is what happens in this case:

1. Subscription.postgresql.cnpg.io is created in kube
2. Subscription is created in PG by CNPG manager
3. It's status is [updated](https://github.com/cloudnative-pg/cloudnative-pg/blob/main/internal/management/controller/common.go#L90 & https://github.com/cloudnative-pg/cloudnative-pg/blob/main/api/v1/subscription_funcs.go#L43)
4. A new reconcile is triggered (sometimes with the old resource present in cache, ie without any `status` field)
5. Because there is no status we bypass [this check](https://github.com/cloudnative-pg/cloudnative-pg/blob/main/internal/management/controller/subscription_controller.go#L83)
6. The subscription is already present in PG, so we trigger the same bad `ALTER` statement described in the issue.

Here are the changes of the kubernetes subscription object:

```diff
@@ ====step 1==== @@

@@ (root level) @@
! ± type change from string to map
- {}
+ apiVersion: postgresql.cnpg.io/v1
  kind: Subscription
  metadata:
    creationTimestamp: "2025-05-26T12:58:46Z"
    generation: 1
    name: xxx
    namespace: preview-532-cnpg-database
    resourceVersion: 841378393
    uid: 5377e60a-14b4-4f37-b228-d70be955f9b1
  spec:
    cluster:
      name: xxx
    dbname: xxx
    externalClusterName: internal-legacy
    name: legacy_replica1
    parameters:
      copy_data: "true"
    publicationName: inter_cluster
    subscriptionReclaimPolicy: delete

@@ ====step 2==== @@

@@ metadata @@
! + one map entry added:
+   finalizers:
+   - cnpg.io/deleteSubscription

@@ metadata.resourceVersion @@
! ± value change
- 841378393
+ 841378394

@@ ====step 3==== @@

@@ (root level) @@
! + one map entry added:
+   status:
+   │ applied: true
+   │ observedGeneration: 1

@@ metadata.resourceVersion @@
! ± value change
- 841378394
+ 841378401

@@ ====step 4==== @@

@@ metadata.resourceVersion @@
! ± value change
- 841378401
+ 841378404

@@ status @@
! + one map entry added:
+   message: "while patching subscription: ERROR: unrecognized subscription parameter: "copy_data" (SQLSTATE 42601)"

@@ status.applied @@
! ± value change
- true
+ false
```

Note: this behavior of the cache seems known: https://github.com/kubernetes-sigs/controller-runtime/issues/1464, and using a simple [`r.Update()` instead of `r.Status().Update()`](https://github.com/kubernetes-sigs/controller-runtime/issues/1464#issuecomment-824351925) seems to fix it

Solution patch:
diff --git a/api/v1/subscription_types.go b/api/v1/subscription_types.go
index 2a4f4f71..adf9f70f 100644
--- a/api/v1/subscription_types.go
+++ b/api/v1/subscription_types.go
@@ -52,8 +52,11 @@ type SubscriptionSpec struct {
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="dbname is immutable"
 	DBName string `json:"dbname"`
 
-	// Subscription parameters part of the `WITH` clause as expected by
-	// PostgreSQL `CREATE SUBSCRIPTION` command
+	// Subscription parameters included in the `WITH` clause of the PostgreSQL
+	// `CREATE SUBSCRIPTION` command. Most parameters cannot be changed
+	// after the subscription is created and will be ignored if modified
+	// later, except for a limited set documented at:
+	// https://www.postgresql.org/docs/current/sql-altersubscription.html#SQL-ALTERSUBSCRIPTION-PARAMS-SET
 	// +optional
 	Parameters map[string]string `json:"parameters,omitempty"`
 
diff --git a/config/crd/bases/postgresql.cnpg.io_subscriptions.yaml b/config/crd/bases/postgresql.cnpg.io_subscriptions.yaml
index 37fdac91..72d7b72f 100644
--- a/config/crd/bases/postgresql.cnpg.io_subscriptions.yaml
+++ b/config/crd/bases/postgresql.cnpg.io_subscriptions.yaml
@@ -93,8 +93,11 @@ spec:
                 additionalProperties:
                   type: string
                 description: |-
-                  Subscription parameters part of the `WITH` clause as expected by
-                  PostgreSQL `CREATE SUBSCRIPTION` command
+                  Subscription parameters included in the `WITH` clause of the PostgreSQL
+                  `CREATE SUBSCRIPTION` command. Most parameters cannot be changed
+                  after the subscription is created and will be ignored if modified
+                  later, except for a limited set documented at:
+                  https://www.postgresql.org/docs/current/sql-altersubscription.html#SQL-ALTERSUBSCRIPTION-PARAMS-SET
                 type: object
               publicationDBName:
                 description: |-
diff --git a/docs/src/cloudnative-pg.v1.md b/docs/src/cloudnative-pg.v1.md
index 08160dba..d3853641 100644
--- a/docs/src/cloudnative-pg.v1.md
+++ b/docs/src/cloudnative-pg.v1.md
@@ -5838,8 +5838,11 @@ the &quot;subscriber&quot; cluster</p>
 <i>map[string]string</i>
 </td>
 <td>
-   <p>Subscription parameters part of the <code>WITH</code> clause as expected by
-PostgreSQL <code>CREATE SUBSCRIPTION</code> command</p>
+   <p>Subscription parameters included in the <code>WITH</code> clause of the PostgreSQL
+<code>CREATE SUBSCRIPTION</code> command. Most parameters cannot be changed
+after the subscription is created and will be ignored if modified
+later, except for a limited set documented at:
+https://www.postgresql.org/docs/current/sql-altersubscription.html#SQL-ALTERSUBSCRIPTION-PARAMS-SET</p>
 </td>
 </tr>
 <tr><td><code>publicationName</code> <B>[Required]</B><br/>
diff --git a/internal/management/controller/subscription_controller.go b/internal/management/controller/subscription_controller.go
index 16450f07..10f06a71 100644
--- a/internal/management/controller/subscription_controller.go
+++ b/internal/management/controller/subscription_controller.go
@@ -42,9 +42,10 @@ type SubscriptionReconciler struct {
 	client.Client
 	Scheme *runtime.Scheme
 
-	instance            *postgres.Instance
-	finalizerReconciler *finalizerReconciler[*apiv1.Subscription]
-	getDB               func(name string) (*sql.DB, error)
+	instance                *postgres.Instance
+	finalizerReconciler     *finalizerReconciler[*apiv1.Subscription]
+	getDB                   func(name string) (*sql.DB, error)
+	getPostgresMajorVersion func() (int, error)
 }
 
 // subscriptionReconciliationInterval is the time between the
@@ -190,6 +191,10 @@ func NewSubscriptionReconciler(
 		getDB: func(name string) (*sql.DB, error) {
 			return instance.ConnectionPool().Connection(name)
 		},
+		getPostgresMajorVersion: func() (int, error) {
+			version, err := instance.GetPgVersion()
+			return int(version.Major), err //nolint:gosec
+		},
 	}
 	sr.finalizerReconciler = newFinalizerReconciler(
 		mgr.GetClient(),
diff --git a/internal/management/controller/subscription_controller_sql.go b/internal/management/controller/subscription_controller_sql.go
index c81a0f5a..89b255fc 100644
--- a/internal/management/controller/subscription_controller_sql.go
+++ b/internal/management/controller/subscription_controller_sql.go
@@ -77,7 +77,11 @@ func (r *SubscriptionReconciler) patchSubscription(
 	obj *apiv1.Subscription,
 	connString string,
 ) error {
-	sqls := toSubscriptionAlterSQL(obj, connString)
+	version, err := r.getPostgresMajorVersion()
+	if err != nil {
+		return fmt.Errorf("while getting the PostgreSQL major version: %w", err)
+	}
+	sqls := toSubscriptionAlterSQL(obj, connString, version)
 	for _, sqlQuery := range sqls {
 		if _, err := db.ExecContext(ctx, sqlQuery); err != nil {
 			return err
@@ -112,7 +116,7 @@ func toSubscriptionCreateSQL(obj *apiv1.Subscription, connString string) string
 	return createQuery
 }
 
-func toSubscriptionAlterSQL(obj *apiv1.Subscription, connString string) []string {
+func toSubscriptionAlterSQL(obj *apiv1.Subscription, connString string, pgMajorVersion int) []string {
 	result := make([]string, 0, 3)
 
 	setPublicationSQL := fmt.Sprintf(
@@ -133,7 +137,7 @@ func toSubscriptionAlterSQL(obj *apiv1.Subscription, connString string) []string
 			fmt.Sprintf(
 				"ALTER SUBSCRIPTION %s SET (%s)",
 				pgx.Identifier{obj.Spec.Name}.Sanitize(),
-				toPostgresParameters(obj.Spec.Parameters),
+				toPostgresParameters(filterSubscriptionUpdatableParameters(obj.Spec.Parameters, pgMajorVersion)),
 			),
 		)
 	}
@@ -141,6 +145,32 @@ func toSubscriptionAlterSQL(obj *apiv1.Subscription, connString string) []string
 	return result
 }
 
+func filterSubscriptionUpdatableParameters(parameters map[string]string, pgMajorVersion int) map[string]string {
+	// Only a limited set of the parameters can be updated
+	// see https://www.postgresql.org/docs/current/sql-altersubscription.html#SQL-ALTERSUBSCRIPTION-PARAMS-SET
+	allowedParameters := []string{
+		"slot_name",
+		"synchronous_commit",
+		"binary",
+		"streaming",
+		"disable_on_error",
+		"password_required",
+		"run_as_owner",
+		"origin",
+		"failover",
+	}
+	if pgMajorVersion >= 18 {
+		allowedParameters = append(allowedParameters, "two_phase")
+	}
+	filteredParameters := make(map[string]string, len(parameters))
+	for _, key := range allowedParameters {
+		if _, present := parameters[key]; present {
+			filteredParameters[key] = parameters[key]
+		}
+	}
+	return filteredParameters
+}
+
 func executeDropSubscription(ctx context.Context, db *sql.DB, name string) error {
 	if _, err := db.ExecContext(
 		ctx,



Test patch:
diff --git a/internal/management/controller/subscription_controller_sql_test.go b/internal/management/controller/subscription_controller_sql_test.go
index d3628858..2678e75c 100644
--- a/internal/management/controller/subscription_controller_sql_test.go
+++ b/internal/management/controller/subscription_controller_sql_test.go
@@ -35,6 +35,8 @@ import (
 
 // nolint: dupl
 var _ = Describe("subscription sql", func() {
+	const defaultPostgresMajorVersion = 17
+
 	var (
 		dbMock sqlmock.Sqlmock
 		db     *sql.DB
@@ -132,28 +134,52 @@ var _ = Describe("subscription sql", func() {
 		}
 		connString := "host=localhost user=test dbname=test"
 
-		sqls := toSubscriptionAlterSQL(obj, connString)
+		sqls := toSubscriptionAlterSQL(obj, connString, defaultPostgresMajorVersion)
 		Expect(sqls).To(ContainElement(`ALTER SUBSCRIPTION "test_sub" SET PUBLICATION "test_pub"`))
 		Expect(sqls).To(ContainElement(`ALTER SUBSCRIPTION "test_sub" CONNECTION 'host=localhost user=test dbname=test'`))
 	})
 
-	It("generates correct SQL for altering subscription with parameters", func() {
+	It("generates correct SQL for altering subscription with parameters for PostgreSQL 17", func() {
 		obj := &apiv1.Subscription{
 			Spec: apiv1.SubscriptionSpec{
 				Name:            "test_sub",
 				PublicationName: "test_pub",
 				Parameters: map[string]string{
-					"param1": "value1",
-					"param2": "value2",
+					"copy_data": "true",
+					"origin":    "none",
+					"failover":  "true",
+					"two_phase": "true",
+				},
+			},
+		}
+		connString := "host=localhost user=test dbname=test"
+
+		sqls := toSubscriptionAlterSQL(obj, connString, 17)
+		Expect(sqls).To(ContainElement(`ALTER SUBSCRIPTION "test_sub" SET PUBLICATION "test_pub"`))
+		Expect(sqls).To(ContainElement(`ALTER SUBSCRIPTION "test_sub" CONNECTION 'host=localhost user=test dbname=test'`))
+		Expect(sqls).To(ContainElement(`ALTER SUBSCRIPTION "test_sub" SET ("failover" = 'true', "origin" = 'none')`))
+	})
+
+	It("generates correct SQL for altering subscription with parameters for PostgreSQL 18", func() {
+		obj := &apiv1.Subscription{
+			Spec: apiv1.SubscriptionSpec{
+				Name:            "test_sub",
+				PublicationName: "test_pub",
+				Parameters: map[string]string{
+					"copy_data": "true",
+					"origin":    "none",
+					"failover":  "true",
+					"two_phase": "true",
 				},
 			},
 		}
 		connString := "host=localhost user=test dbname=test"
 
-		sqls := toSubscriptionAlterSQL(obj, connString)
+		sqls := toSubscriptionAlterSQL(obj, connString, 18)
 		Expect(sqls).To(ContainElement(`ALTER SUBSCRIPTION "test_sub" SET PUBLICATION "test_pub"`))
 		Expect(sqls).To(ContainElement(`ALTER SUBSCRIPTION "test_sub" CONNECTION 'host=localhost user=test dbname=test'`))
-		Expect(sqls).To(ContainElement(`ALTER SUBSCRIPTION "test_sub" SET ("param1" = 'value1', "param2" = 'value2')`))
+		Expect(sqls).To(ContainElement(
+			`ALTER SUBSCRIPTION "test_sub" SET ("failover" = 'true', "origin" = 'none', "two_phase" = 'true')`))
 	})
 
 	It("returns correct SQL for altering subscription with no owner or parameters", func() {
@@ -165,7 +191,7 @@ var _ = Describe("subscription sql", func() {
 		}
 		connString := "host=localhost user=test dbname=test"
 
-		sqls := toSubscriptionAlterSQL(obj, connString)
+		sqls := toSubscriptionAlterSQL(obj, connString, defaultPostgresMajorVersion)
 		Expect(sqls).To(ContainElement(`ALTER SUBSCRIPTION "test_sub" SET PUBLICATION "test_pub"`))
 		Expect(sqls).To(ContainElement(`ALTER SUBSCRIPTION "test_sub" CONNECTION 'host=localhost user=test dbname=test'`))
 	})
diff --git a/internal/management/controller/subscription_controller_test.go b/internal/management/controller/subscription_controller_test.go
index 99a2dd1f..343dd4e4 100644
--- a/internal/management/controller/subscription_controller_test.go
+++ b/internal/management/controller/subscription_controller_test.go
@@ -50,6 +50,8 @@ const subscriptionDetectionQuery = `SELECT count(*)
 		WHERE subname = $1`
 
 var _ = Describe("Managed subscription controller tests", func() {
+	const defaultPostgresMajorVersion = 17
+
 	var (
 		dbMock       sqlmock.Sqlmock
 		db           *sql.DB
@@ -123,6 +125,9 @@ var _ = Describe("Managed subscription controller tests", func() {
 			getDB: func(_ string) (*sql.DB, error) {
 				return db, nil
 			},
+			getPostgresMajorVersion: func() (int, error) {
+				return defaultPostgresMajorVersion, nil
+			},
 		}
 		r.finalizerReconciler = newFinalizerReconciler(
 			fakeClient,