Skip to content

K8SPG-1073: Remove restore_command when backups are disabled#1673

Open
yoav-katz wants to merge 6 commits into
percona:mainfrom
yoav-katz:bug/remove-remote_command
Open

K8SPG-1073: Remove restore_command when backups are disabled#1673
yoav-katz wants to merge 6 commits into
percona:mainfrom
yoav-katz:bug/remove-remote_command

Conversation

@yoav-katz

Copy link
Copy Markdown

CHANGE DESCRIPTION

Problem:
When spec.backups.enabled: false is set, the operator still configures Patroni's restore_command to invoke pgBackRest (pgbackrest --stanza=<name> archive-get %f "%p"), even though no backup repository exists. Any time a replica needs a WAL segment it can't get via streaming replication, Postgres calls this command, which fails because there's no valid stanza/repo — instead of falling back cleanly to "no WAL available from archive."

Cause:
internal/pgbackrest/postgres.go's PostgreSQL() function already neutralizes archive_command when backups are disabled (sets it to a no-op true), but the equivalent handling was missing for restore_command — it was built and added to the Patroni mandatory parameter set unconditionally, with no backupsEnabled check at all.

Solution:
restore_command is no longer set when backups are disabled. Unlike archive_command, it can't simply be pointed at a no-op placeholder like true: Postgres treats a zero exit status from restore_command as "the file was placed at %p," so a placeholder that always succeeds would make Postgres believe WAL recovery succeeded when it didn't, risking a crash or silent gap. Instead, restore_command is omitted entirely in this case, so Postgres relies purely on streaming replication and local WAL. An explicit user override of restore_command via spec.patroni.dynamicConfiguration is still respected regardless of backups.enabled, since that reflects an intentional choice by the user.

CHECKLIST

Jira

  • Is the Jira ticket created and referenced properly?
  • Does the Jira ticket have the proper statuses for documentation (Needs Doc) and QA (Needs QA)?
  • Does the Jira ticket link to the proper milestone (Fix Version field)?

Tests

  • Is an E2E test/test case added for the new feature/change?
  • Are unit tests added where appropriate?

Config/Logging/Testability

  • Are all needed new/changed options added to default YAML files?
  • Are all needed new/changed options added to the Helm Chart?
  • Did we add proper logging messages for operator actions?
  • Did we ensure compatibility with the previous version or cluster upgrade process?
  • Does the change support oldest and newest supported PG version?
  • Does the change support oldest and newest supported Kubernetes version?

@egegunes egegunes changed the title Remove restore_command when backups are disabled K8SPG-1073: Remove restore_command when backups are disabled Jul 6, 2026
@egegunes egegunes added this to the v3.1.0 milestone Jul 6, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates pgBackRest-related PostgreSQL parameter generation so Patroni does not configure restore_command when backups are disabled, preventing Postgres from attempting (and failing) WAL archive restores when no repository exists.

Changes:

  • Omit restore_command from Patroni mandatory parameters when backups are disabled, unless the user explicitly overrides it.
  • Add unit test coverage for the “backups disabled” behavior, including override and standby-repo scenarios.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
internal/pgbackrest/postgres.go Conditionally adds restore_command only when backups are enabled or user-overridden (standby repo still forces it).
internal/pgbackrest/postgres_test.go Adds a test case ensuring restore_command is omitted when backups are disabled, while preserving override and standby behavior.

Comment on lines +104 to +106
// didn't. Leave restore_command unset so Postgres relies on streaming
// replication and local WAL only -- unless the user explicitly configured
// their own restore_command.
@egegunes

Copy link
Copy Markdown
Contributor

@yoav-katz monitoring tests are failing repeatedly but I doubt it's related to your changes. we'll check.

@yoav-katz

Copy link
Copy Markdown
Author

the same tests are failing in another PR I opened #1647
the cause of the failure -
error creating gce static IP address: googleapi: Error 403: QUOTA_EXCEEDED - Quota

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants