Skip to content

pg templates: correct the README failover timings and ship the backup warning that missed #478 - #479

Merged
jacobecox merged 2 commits into
mainfrom
claude/pg-audit-round2
Aug 24, 2026
Merged

pg templates: correct the README failover timings and ship the backup warning that missed #478#479
jacobecox merged 2 commits into
mainfrom
claude/pg-audit-round2

Conversation

@jacobecox

Copy link
Copy Markdown
Contributor

What

Follow-up to #478, from the in-depth test session you asked for. Everything here is in place — README and lastModified only — with helm template output verified byte-identical on all three charts, so no version is bumped.

Two corrections, both to problems the #478 work itself created:

1. The empty-object warning never shipped. It was still uncommitted when #478 was squash-merged, so it isn't in main. Caught by diffing the branch against main rather than trusting that "merged" meant everything landed.

2. The README timings went stale. Retuning the DCS values to 45/10/15 left the tables advertising ttl 60s / retry_timeout 20s — and the patronictl edit-config examples still told users to set 60 / 10 / 20, which would move a cluster away from what ships. That is the exact README-vs-values drift this audit has been closing everywhere else, introduced by the fix for it.

The ttl claim was not supported by measurement

The tables said ttl "sets the worst-case failover delay". Measured on a 3-member cluster:

ttl 60 / fall 3 ttl 45 / fall 2
first failed write +19s +15s
recovery complete +69s +70s

Lowering ttl did not move abrupt recovery. What did move was the graceful case — 6 failed writes over ~7s became 3 over ~4s — and that came from the HAProxy fall 3 → 2 change, not from ttl. Patroni's own handover is 185 ms; clients wait on the health check.

Replaced with measured behaviour, and stated as measured rather than derived because the method has a real limit: the abrupt case is simulated by SIGSTOP-ing Patroni's worker (PID 1 is signal-immune from inside the container), PID 1 stays alive, and there was one run per configuration. So the honest claim is that ttl was not the dominant term in this test.

Test session results (against the merged 2.6.0 / 1.2.0 / 1.1.0)

Area Result
failsafe_mode holds through DCS process death (129s) and true network partition (330s) — zero refused writes; A/B proved the 39s recovery gap was an artifact of restarting etcd, not inherent
Backups all four provider/mode combos verified with real restores — MinIO 42M, AWS 30M, GCP 34M PGDATA; logical = valid 163-line dump
multiZone replicas genuinely spread across us-east-1a / 1b / 1c
replicas: 1 valid single-member cluster
etcd.replicas: 1 correctly rejected at render — quorum enforced
internal_access: workload-list enforcing — proven with positive and negative controls
Drift gate pg-ha 13/13, timescaledb 12/12, multi-location 13/13 Unchanged, zero Updated
multi-location deployed across 3 regions, drift-clean, teardown took its created GVC with it

Not covered, stated plainly

  • postgres-multi-location had no in-container verification. cpln workload exec into the GVC it creates is denied by the policy hook; workload get reads are permitted, exec is not. So no patronictl, no runtime confirmation that failsafe_mode is active, no credential round-trip. Its functional changes are the same values proven end-to-end on the other two, but this is not the same standard of evidence.
  • The empty-object upload bug belongs to backup-images, not this repo. This PR only adds the warning so nobody restores a zero-length dump meanwhile.

🤖 Generated with Claude Code

jacobecox and others added 2 commits August 24, 2026 14:50
… is a failed run

Fixed IN PLACE - README and lastModified only, so `helm template` output is
byte-identical before and after on both charts (verified).

Testing found that a failed logical backup still leaves a ~20-byte empty gzip in
the bucket under a normal-looking timestamped filename, because the upload is a
pipeline:

    pg_dumpall ... | gzip | aws s3 cp - "s3://.../${FILENAME}"

`set -euo pipefail` makes the job exit non-zero, but gzip emits a valid empty
stream on EOF and the upload completes before the shell reacts. Measured: the bad
object gunzips to 0 lines, a good one from the same cluster to 163 lines with
CREATE ROLE statements. Nothing distinguishes them but size.

The real fix belongs in the backup-images repo (dump to a temp file, verify it is
non-empty, then upload) and is being raised there. This is the template-side
mitigation so nobody restores an empty dump in the meantime.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hange left stale

Fixed IN PLACE - README and lastModified only; `helm template` output verified
byte-identical on all three charts.

Two problems, both introduced when the DCS timeouts were retuned to 45/10/15 and the
READMEs were not updated with them:

The tables still advertised ttl 60s and retry_timeout 20s, and the patronictl
edit-config examples still told users to set 60 / 10 / 20 - which would have moved a
cluster AWAY from what the chart now ships. That is the same README-vs-values drift
this audit has been closing everywhere else, introduced by the fix for it.

The tables also claimed ttl "sets the worst-case failover delay". Measurement does not
support that. Abrupt recovery took ~70s at ttl 60 and ~70s at ttl 45 - lowering ttl did
not move it. What did move was the graceful case, from ~7s to ~4s, and that came from
the HAProxy fall 3 -> 2 change rather than from ttl.

Replaced with what was actually measured: a planned failover releases the lock
immediately and costs a few seconds, with Patroni's own handover at 185ms and the
HAProxy health check being what clients wait on; an abrupt loss takes about a minute,
and ttl is the lock-expiry bound rather than a dial for failover speed.

Stated as measured rather than derived, because the abrupt case is simulated by
SIGSTOPing Patroni's worker (PID 1 is signal-immune from inside the container), PID 1
stays alive, and there was one run per configuration. That may not isolate the ttl term,
so the honest claim is that ttl was not the dominant term in this test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jacobecox
jacobecox merged commit a420461 into main Aug 24, 2026
6 checks passed
@jacobecox
jacobecox deleted the claude/pg-audit-round2 branch August 24, 2026 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant