Never emit an empty synchronous_standby_names list - #73
Open
souravbiswassanto wants to merge 1 commit into
Open
souravbiswassanto wants to merge 1 commit into
souravbiswassanto wants to merge 1 commit into
Conversation
The role scripts build synchronous_standby_names from $REPLICAS, skipping
the pod's own index. At one replica that list is empty, and the emitted
value becomes 'FIRST 2 ()' -- with the mode and quorum still coming from
SYNC_REPLICATION_MODE/NUM_SYNC_REPLICAS, which describe the eventual
topology rather than the current one.
Postgres does not read an empty list as "no synchronous standbys". It is a
parse error:
LOG: invalid value for parameter "synchronous_standby_names": "FIRST 2 ()"
DETAIL: syntax error at or near ")"
FATAL: configuration file "/var/pv/data/postgresql.conf" contains errors
so the server never starts. scripts/run.sh then re-runs the role script on
its one-second loop and start.sh prepends another block each pass, so the
file also accumulates duplicate entries while the pod sits not-ready.
This is reachable today: a PITR restore with replicationStrategy sync or
fscopy -- the default for postgres -- collapses the CR to a single replica
while leaving streamingMode Synchronous, so any synchronous cluster with
replicas > 1 fails to come back from a restore.
Fall back to an empty string when the list is empty. Postgres accepts
synchronous_standby_names = '' and treats every synchronous_commit level as
local, so commits wait for local flush only: no error and no hang. The next
reconcile rewrites the file with a real list once the peers exist.
Verified against postgres 16.15: 'FIRST 2 ()' reproduces the error above,
'' and 'FIRST 2 ("a","b")' both start.
Applied to all 40 emitters (majors 9-18 x primary/start, standby/run,
standby/warm_stanby, standby/ha_backup_job); the majors are independent
copies, so each needs the guard. No behaviour change when the list is
non-empty.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: souravbiswassanto <saurov@appscode.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The role scripts build
synchronous_standby_namesfrom$REPLICAS, skipping the pod's own index:At one replica that loop skips its only iteration and
$namesis empty, whileSYNC_REPLICATION_MODE/NUM_SYNC_REPLICASstill come from the CR and describe the eventualtopology. The emitted value is
'FIRST 2 ()'.Postgres does not read an empty list as "no synchronous standbys" — it is a parse error, and the
server refuses to start:
scripts/run.shthen re-runs the role script on its one-second loop, andstart.shprepends afresh block each pass, so the config also accumulates duplicate entries while the pod sits
not-ready:
How it is reached today
A PITR restore.
postgres/pkg/controller/restore.gocollapses the CR to a single replica for thesyncandfscopystrategies, while leavingstreamingMode: Synchronousin place:syncis the default (postgres_helpers.goSetDefaults), so this affects any synchronouscluster with
replicas > 1being restored — the user need not have chosen a strategy at all. Therestore's WAL replay completes and then the primary never starts; the CR sits in
Provisioning.Reported against KubeDB v2026.1.19 with PG 16.9,
streamingMode: Synchronous,synchronousReplicationConfig: {mode: First, numSyncReplicas: 2},replicas: 3.Fix
Guard the empty list. Postgres accepts
synchronous_standby_names = ''and treats everysynchronous_commitlevel aslocal, so commits wait for local flush only — no error, and nohang waiting for standbys that do not exist. The next reconcile rewrites the file with a real list
once the peers are up.
Applied to all 40 emitters (majors 9–18 ×
primary/start.sh,standby/run.sh,standby/warm_stanby.sh,standby/ha_backup_job.sh). The majors are independent copies, so eachone needs the guard. No behaviour change when the list is non-empty.
Verification
Against a real PostgreSQL 16.15:
'FIRST 2 ()'invalid value for parameter ... syntax error at or near ")"— reproduces the report exactly'''FIRST 2 ("a","b")'Replaying the block from
role_scripts/16/primary/start.sh:bash -npasses on all 66 scripts inrole_scripts/.shfmt -l -ci -i 4reports the same 56 files before and after, so this adds no formatting drift.Related
https://github.com/kubedb/postgres/pull/938 removes the mismatch at the source by restoring asynchronously while the CR is
collapsed to one pod. Either change alone fixes the reported case; this one is the defensive half
and covers every other path that can produce an empty list.
Shipping this to existing clusters needs a
postgres-initrelease plus aPostgresVersioncatalog bump in kubedb/installer.
🤖 Generated with Claude Code