From 2af306e2756eaa0d8605c526ca982bf2f9353c05 Mon Sep 17 00:00:00 2001 From: Paulo Cabral Sanz Date: Fri, 9 Oct 2026 14:05:15 -0300 Subject: [PATCH] pgbackrest: one backup reader and a spread start checkpoint by default; --start-fast when it would outlast db-timeout Backups competed with live queries for the same volume: process-max for backup was clamp(cpus/4, 1, 2) and start-fast=y forced an immediate checkpoint at every backup start. Default backup process-max to 1 at every vCPU size and start-fast=n, mirroring postgres-ssl #151. Explicit PGBACKREST_BACKUP_PROCESS_MAX and PGBACKREST_START_FAST keep winning. A spread backup-start checkpoint waits about checkpoint_completion_target x checkpoint_timeout however little is dirty (measured on PG 16: 275 MB dirty, checkpoint_timeout=60s -> 55 s; 0.5 s with fast), about twice that when a spread checkpoint is already running. pgBackRest bounds pg_backup_start with db-timeout (1800 s) and the stall watchdog sees no byte progress meanwhile, so a checkpoint_timeout of roughly 17 min or more would fail every backup before it copies anything. Before each backup the watcher now reads both settings and, when the worst case reaches the lower of db-timeout (PGBACKREST_DB_TIMEOUT, s/m/h suffixes) and WAL_BACKUP_STALL_SECONDS, adds --start-fast and logs why. An operator-set PGBACKREST_START_FAST is left alone; a failed settings query changes nothing. Mirrors postgres-ssl #155. Tests: process_max_defaults is pure and asserted at 1-256 vCPU; the rendered conf is asserted to carry start-fast=n (both fail on the previous defaults); start_fast_tests cover db-timeout parsing, the limit and the threshold. The e2e asserts start-fast config/env precedence on the initial full and adds a cluster with checkpoint_timeout=20min that must log the --start-fast decision, then one with PGBACKREST_START_FAST=n that must not. --- README.md | 20 ++- .../src/patroni/backup_watcher.rs | 170 +++++++++++++++++- postgres-patroni/src/wal_archive.rs | 70 +++++++- test/e2e-ha.sh | 62 +++++++ 4 files changed, 309 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 09bb42c7..2fe25716 100644 --- a/README.md +++ b/README.md @@ -98,8 +98,9 @@ Image-level tuning knobs: | `WAL_DROP_THRESHOLD_MB` | `pg_wal/` size at which the archive-push wrapper drops failing segments to keep Postgres running (default `5120`, matching `archive-push-queue-max`). Outside the `PGBACKREST_*` namespace because pgBackRest treats unknown `PGBACKREST_*` vars as config options and warns about them on every push. | | `PGBACKREST_ARCHIVE_PUSH_PROCESS_MAX` | parallel workers for `archive-push`. Default auto-sized as `clamp(cpus/8, 2, 8)`. | | `PGBACKREST_ARCHIVE_GET_PROCESS_MAX` | parallel workers for `archive-get`. Default auto-sized as `clamp(cpus/8, 2, 8)`. | -| `PGBACKREST_BACKUP_PROCESS_MAX` | parallel workers for `backup`. Default auto-sized as `clamp(cpus/4, 1, 2)`. Set `1` to reduce backup concurrency independently of archiving and restores. | +| `PGBACKREST_BACKUP_PROCESS_MAX` | parallel workers for `backup`. Default `1`, independent of vCPU, to reduce contention with live queries on IOPS-limited volumes. Explicit overrides still win. | | `PGBACKREST_RESTORE_PROCESS_MAX` | parallel workers for `restore`. Default auto-sized as `clamp(cpus, 1, 32)`. | +| `PGBACKREST_START_FAST` | native pgBackRest option. Default `n` in the image config: spread the backup-start checkpoint instead of forcing a fast one. Set `y` to opt into the previous behavior. Setting it either way also turns off the watcher's own decision below. | The four worker overrides are template settings, not native pgBackRest environment options. `patroni-runner` renders them into command-specific @@ -108,6 +109,23 @@ config sections. The image's `pgbackrest` launcher removes these variables environment so unknown-option warnings cannot corrupt `info --output=json`. Native options such as `PGBACKREST_REPO1_PATH` remain available. +Full and differential backups use one worker and `start-fast=n` by default. +This reduces reader concurrency and checkpoint write bursts; it is not a hard +IOPS cap, and backups can take longer to start and finish. WAL shipping, +archive-get, restore worker counts, backup cadence and retention are +unchanged. Redeploy to pick up the new defaults. + +Before each backup the watcher reads `checkpoint_timeout` and +`checkpoint_completion_target` from the server. A backup opened without +`start-fast` waits for a spread checkpoint, up to about twice +`checkpoint_completion_target × checkpoint_timeout` when one is already +running, with no byte progress for the stall watchdog to see and under +pgBackRest's 30-minute `db-timeout`. When that worst case reaches the lower +of the two, the watcher adds `--start-fast` (one immediate checkpoint) and +logs why, instead of letting every backup time out. `PGBACKREST_START_FAST` +(`y` or `n`) turns the heuristic off; pgBackRest reads that variable directly +and it wins over the config file. + When `WAL_ARCHIVE_BUCKET` is set, `patroni-runner` writes `archive_mode=on`, `archive_command='/usr/local/bin/pgbackrest-archive-push-wrapper.sh %p'`, and `archive_timeout` (default `60`, override via `POSTGRES_ARCHIVE_TIMEOUT`) diff --git a/postgres-patroni/src/patroni/backup_watcher.rs b/postgres-patroni/src/patroni/backup_watcher.rs index adb561fe..642be70a 100644 --- a/postgres-patroni/src/patroni/backup_watcher.rs +++ b/postgres-patroni/src/patroni/backup_watcher.rs @@ -209,7 +209,8 @@ struct StallConfig { /// `WAL_BACKUP_STALL_SECONDS` (default 1800 = 30 min; 0 disables the /// watchdog): the floor of the window. It covers the phases in which /// pgBackRest reports no byte progress at all — pg_backup_start - /// (start-fast=y: one immediate checkpoint), removing a non-resumable + /// (start-fast=n: a spread checkpoint, or an immediate one when + /// `backup_start_fast_arg` adds --start-fast), removing a non-resumable /// earlier attempt from the bucket, building and saving the manifest, and /// the tail after the last progress write (pg_backup_stop plus the /// archive-timeout wait for the closing WAL). Seconds to minutes on a @@ -244,6 +245,117 @@ impl StallConfig { } } +/// pgBackRest's `db-timeout` default, in seconds; it bounds `pg_backup_start`. +const PGBACKREST_DB_TIMEOUT_DEFAULT_SECONDS: u64 = 1800; + +/// `PGBACKREST_DB_TIMEOUT` as seconds: a plain number or one with an s/m/h +/// suffix (what pgBackRest accepts). Anything else is `None`. +fn parse_db_timeout_seconds(raw: Option<&str>) -> Option { + let raw = raw?.trim(); + let (num, mult) = if let Some(n) = raw.strip_suffix('h') { + (n, 3600) + } else if let Some(n) = raw.strip_suffix('m') { + (n, 60) + } else { + (raw.strip_suffix('s').unwrap_or(raw), 1) + }; + num.parse::().ok().map(|n| n * mult) +} + +/// Bound the backup-start wait must stay under: pgBackRest's db-timeout or +/// the stall watchdog floor, whichever is lower. A disabled watchdog (0) +/// does not bound it. +fn backup_start_wait_limit_seconds(db_timeout_env: Option<&str>, stall_floor_seconds: u64) -> u64 { + let db_timeout = + parse_db_timeout_seconds(db_timeout_env).unwrap_or(PGBACKREST_DB_TIMEOUT_DEFAULT_SECONDS); + if stall_floor_seconds > 0 { + db_timeout.min(stall_floor_seconds) + } else { + db_timeout + } +} + +/// Pure verdict: a spread backup-start checkpoint that could wait +/// `wait_seconds` needs `--start-fast` under a `limit_seconds` bound. +fn needs_start_fast(wait_seconds: u64, limit_seconds: u64) -> bool { + wait_seconds >= limit_seconds +} + +/// Worst-case seconds `pg_backup_start(fast => false)` can wait on this +/// server: 2 × checkpoint_completion_target × checkpoint_timeout, rounded +/// up. Without start-fast Postgres spreads the backup-start checkpoint over +/// completion_target × checkpoint_timeout however little is dirty (measured +/// on 16: 275 MB dirty, checkpoint_timeout=60s → 55 s wait, 0.5 s with +/// fast), and a request that lands while a spread checkpoint is already +/// running waits for that one and then for its own. +async fn spread_checkpoint_wait_seconds() -> Result { + let out = Command::new("psql") + .args([ + "-U", + "postgres", + "-h", + "/var/run/postgresql", + "-tAXq", + "-c", + "SELECT ceil(2 * t.setting::numeric * c.setting::numeric)::bigint \ + FROM pg_settings t, pg_settings c \ + WHERE t.name = 'checkpoint_timeout' \ + AND c.name = 'checkpoint_completion_target'", + ]) + .env_remove("PGHOST") + .env_remove("PGPORT") + .output() + .await?; + if !out.status.success() { + anyhow::bail!( + "checkpoint settings query failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + } + let text = String::from_utf8_lossy(&out.stdout); + text.trim() + .parse::() + .map_err(|e| anyhow::anyhow!("checkpoint settings query returned {:?}: {e}", text.trim())) +} + +/// Extra `pgbackrest backup` argument for how the backup is opened. +/// +/// `pg_backup_start` runs under pgBackRest's db-timeout (1800 s) and the +/// stall watchdog sees no byte progress while it waits for the checkpoint, +/// so a checkpoint_timeout long enough to push the worst case past either +/// limit would make every backup fail before it copies a byte. When the +/// server's settings say so this returns `--start-fast` (one immediate +/// checkpoint) and logs why. An operator who set `PGBACKREST_START_FAST` has +/// decided already: pgBackRest reads that variable directly and it wins +/// over the config file, so the watcher stays out of it. Unknown settings +/// (a failed query) change nothing. Mirrors postgres-ssl's +/// `decide_backup_start_fast`. +async fn backup_start_fast_arg(cfg: &StallConfig) -> Option<&'static str> { + if env::var_os("PGBACKREST_START_FAST").is_some() { + return None; + } + let wait = match spread_checkpoint_wait_seconds().await { + Ok(w) => w, + Err(e) => { + debug!(error = %e, "pgbackrest-watcher: backup start: checkpoint settings unavailable; starting as configured"); + return None; + } + }; + let limit = backup_start_wait_limit_seconds( + env::var("PGBACKREST_DB_TIMEOUT").ok().as_deref(), + cfg.floor_seconds, + ); + if !needs_start_fast(wait, limit) { + return None; + } + info!( + wait_seconds = wait, + limit_seconds = limit, + "pgbackrest-watcher: backup start: a spread checkpoint could wait up to {wait}s (2 x checkpoint_completion_target x checkpoint_timeout), reaching the {limit}s backup-start limit; starting with --start-fast (set PGBACKREST_START_FAST=y or n to decide explicitly)" + ); + Some("--start-fast") +} + impl WatcherConfig { fn from_env() -> Self { let full_hours = env_u64("WAL_BACKUP_FULL_INTERVAL_HOURS", 168); @@ -2646,13 +2758,18 @@ async fn run_backup_supervised( backup_type: &str, cfg: &StallConfig, ) -> std::io::Result { + let type_arg = format!("--type={backup_type}"); + let mut args = vec![ + "--stanza=main", + "backup", + type_arg.as_str(), + "--no-expire-auto", + ]; + if let Some(flag) = backup_start_fast_arg(cfg).await { + args.push(flag); + } let mut child = Command::new("pgbackrest") - .args([ - "--stanza=main", - "backup", - &format!("--type={backup_type}"), - "--no-expire-auto", - ]) + .args(&args) .env_remove("PGHOST") .env_remove("PGPORT") .spawn()?; @@ -4062,6 +4179,45 @@ P00 INFO: stanza-create command end: aborted with exception [055]\n"; } } +#[cfg(test)] +mod start_fast_tests { + use super::{backup_start_wait_limit_seconds, needs_start_fast, parse_db_timeout_seconds}; + + #[test] + fn db_timeout_parses_seconds_and_suffixes() { + assert_eq!(parse_db_timeout_seconds(Some("1800")), Some(1800)); + assert_eq!(parse_db_timeout_seconds(Some("900s")), Some(900)); + assert_eq!(parse_db_timeout_seconds(Some("45m")), Some(2700)); + assert_eq!(parse_db_timeout_seconds(Some("2h")), Some(7200)); + assert_eq!(parse_db_timeout_seconds(None), None); + assert_eq!(parse_db_timeout_seconds(Some("")), None); + assert_eq!(parse_db_timeout_seconds(Some("m")), None); + assert_eq!(parse_db_timeout_seconds(Some("1800.5")), None); + } + + #[test] + fn limit_is_the_lower_of_db_timeout_and_stall_floor() { + assert_eq!(backup_start_wait_limit_seconds(None, 1800), 1800); + assert_eq!(backup_start_wait_limit_seconds(None, 600), 600); + assert_eq!(backup_start_wait_limit_seconds(None, 0), 1800); + assert_eq!(backup_start_wait_limit_seconds(Some("1h"), 0), 3600); + assert_eq!(backup_start_wait_limit_seconds(Some("1h"), 1800), 1800); + assert_eq!(backup_start_wait_limit_seconds(Some("junk"), 0), 1800); + } + + #[test] + fn start_fast_once_the_spread_wait_reaches_the_limit() { + // 2 x 0.9 x checkpoint_timeout: 5min -> 540, 16min -> 1728, 17min -> 1836. + assert!(!needs_start_fast(540, 1800)); + assert!(!needs_start_fast(1728, 1800)); + assert!(needs_start_fast(1836, 1800)); + assert!(needs_start_fast(2160, 1800)); + // A 600 s stall floor bounds a 400s checkpoint_timeout (720) but not 5min. + assert!(!needs_start_fast(540, 600)); + assert!(needs_start_fast(720, 600)); + } +} + #[cfg(test)] mod stall_tests { use super::{ diff --git a/postgres-patroni/src/wal_archive.rs b/postgres-patroni/src/wal_archive.rs index 02160826..bf99afcf 100644 --- a/postgres-patroni/src/wal_archive.rs +++ b/postgres-patroni/src/wal_archive.rs @@ -283,6 +283,30 @@ pub fn env_or_clamp(var: &str, default: u32) -> u32 { parse_process_max(env::var(var).ok().as_deref(), default) } +/// Per-command `process-max` defaults for a container with `cpus` vCPU, +/// before the `PGBACKREST_*_PROCESS_MAX` overrides. Pure so the sizing is +/// unit-testable without touching env. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct ProcessMaxDefaults { + pub push: u32, + pub get: u32, + pub backup: u32, + pub restore: u32, +} + +/// archive-push / archive-get: `clamp(cpus/8, 2, 8)`. backup: one reader, +/// whatever the vCPU count — volume IOPS do not scale with vCPU and every +/// extra reader competes with live queries for the same disk (mirrors +/// postgres-ssl #151). restore: the DB is down, so `clamp(cpus, 1, 32)`. +pub fn process_max_defaults(cpus: i64) -> ProcessMaxDefaults { + ProcessMaxDefaults { + push: clamp(cpus / 8, 2, 8), + get: clamp(cpus / 8, 2, 8), + backup: 1, + restore: clamp(cpus, 1, 32), + } +} + /// pg_wal drop ceiling (MiB) and pgBackRest archive-push spool ceiling (MiB). /// Both scale DOWN from the absolute default (5120) on small volumes — never /// up. On volumes ≥10 GiB the absolute holds. @@ -345,10 +369,11 @@ pub fn render_pgbackrest_conf(data_dir: &str, queue_max_mib: u32) -> Result<()> let conf_path = "/etc/pgbackrest/pgbackrest.conf"; let cpus = detect_cpus().max(1) as i64; - let push_max = env_or_clamp("PGBACKREST_ARCHIVE_PUSH_PROCESS_MAX", clamp(cpus / 8, 2, 8)); - let get_max = env_or_clamp("PGBACKREST_ARCHIVE_GET_PROCESS_MAX", clamp(cpus / 8, 2, 8)); - let backup_max = env_or_clamp("PGBACKREST_BACKUP_PROCESS_MAX", clamp(cpus / 4, 1, 2)); - let restore_max = env_or_clamp("PGBACKREST_RESTORE_PROCESS_MAX", clamp(cpus, 1, 32)); + let defaults = process_max_defaults(cpus); + let push_max = env_or_clamp("PGBACKREST_ARCHIVE_PUSH_PROCESS_MAX", defaults.push); + let get_max = env_or_clamp("PGBACKREST_ARCHIVE_GET_PROCESS_MAX", defaults.get); + let backup_max = env_or_clamp("PGBACKREST_BACKUP_PROCESS_MAX", defaults.backup); + let restore_max = env_or_clamp("PGBACKREST_RESTORE_PROCESS_MAX", defaults.restore); info!( cpus = cpus, @@ -442,7 +467,7 @@ fn build_pgbackrest_conf(params: &PgbackrestConfParams) -> String { spool-path={spool_dir}\n\ compress-type=zst\n\ compress-level=3\n\ - start-fast=y\n\ + start-fast=n\n\ \n\ [global:archive-push]\n\ process-max={push_max}\n\ @@ -736,6 +761,41 @@ mod tests { } } + #[test] + fn backup_process_max_defaults_to_one_at_every_cpu_size() { + // Volume IOPS do not scale with vCPU: the backup reader count stays + // at 1 while the other commands keep their cpu-derived defaults. + for cpus in [1i64, 4, 16, 64, 256] { + let d = process_max_defaults(cpus); + assert_eq!(d.backup, 1, "cpus={cpus}"); + assert_eq!(d.push, clamp(cpus / 8, 2, 8), "cpus={cpus}"); + assert_eq!(d.get, clamp(cpus / 8, 2, 8), "cpus={cpus}"); + assert_eq!(d.restore, clamp(cpus, 1, 32), "cpus={cpus}"); + } + // Explicit overrides still win and stay command-scoped. + assert_eq!( + parse_process_max(Some("4"), process_max_defaults(256).backup), + 4 + ); + } + + #[test] + fn pgbackrest_conf_spreads_the_backup_start_checkpoint() { + let conf = build_pgbackrest_conf(&PgbackrestConfParams { + data_dir: "/pgdata", + queue_max_mib: 5120, + push_max: 2, + get_max: 2, + backup_max: 1, + restore_max: 4, + retention_full: 4, + retention_diff: 14, + }); + assert!(conf.contains("[global]\n")); + assert!(conf.contains("\nstart-fast=n\n"), "{conf}"); + assert!(!conf.contains("start-fast=y"), "{conf}"); + } + #[test] fn pgbackrest_conf_renders_all_five_process_max_sections() { let conf = build_pgbackrest_conf(&PgbackrestConfParams { diff --git a/test/e2e-ha.sh b/test/e2e-ha.sh index 1c971cb9..0734a1ba 100755 --- a/test/e2e-ha.sh +++ b/test/e2e-ha.sh @@ -1147,6 +1147,8 @@ t_watcher_initial_full() { fi if ! docker exec -u postgres "$leader" bash -e -o pipefail -c "$(_pgbackrest_env_preamble) pgbackrest --stanza=main info --output=json | python3 -m json.tool >/dev/null + env PGBACKREST_START_FAST=y pgbackrest help backup start-fast | grep -Fx \"current: true\" + env -u PGBACKREST_START_FAST pgbackrest help backup start-fast | grep -Fx \"current: false\" for setting in backup:1 archive-push:3 archive-get:3 restore:24; do output=\$(pgbackrest help \"\${setting%:*}\" process-max) grep -F \"current: \${setting#*:}\" <<<\"\$output\" @@ -1162,6 +1164,65 @@ t_watcher_initial_full() { teardown_scope "$scope" } +# The watcher adds --start-fast when a spread backup-start checkpoint could +# reach pgBackRest's db-timeout (2 x completion_target x checkpoint_timeout, +# see backup_start_fast_arg), and leaves the decision to the operator when +# PGBACKREST_START_FAST is set. +t_backup_start_fast_when_checkpoint_outlasts_db_timeout() { + local t=t_backup_start_fast_when_checkpoint_outlasts_db_timeout + local scope=t-start-fast-${PG_VERSION} + local line="backup start: a spread checkpoint could wait up to 2160s" + local n1 n2 n3 leader etcd_hosts + for mode in heuristic explicit; do + reset_bucket + etcd_hosts=$(setup_etcd_cluster "$scope") + # shellcheck disable=SC2046 + if [ "$mode" = explicit ]; then + read -r n1 n2 n3 < <(setup_patroni_cluster "$scope" "$etcd_hosts" $(archive_env_fast_watcher) \ + -e PGBACKREST_START_FAST=n) + else + read -r n1 n2 n3 < <(setup_patroni_cluster "$scope" "$etcd_hosts" $(archive_env_fast_watcher)) + fi + leader=$(wait_for_leader "$scope" 240) || { + ko "$t" "no leader ($mode)" + fail_dump "$t" "$n1" "$n2" "$n3" + teardown_scope "$scope" + return + } + # 20min x 0.9 x 2 = 2160s > 1800s db-timeout. Reloadable, so no restart. + psql_leader "$leader" -c "ALTER SYSTEM SET checkpoint_timeout = '20min';" \ + -c "SELECT pg_reload_conf();" >/dev/null + wait_for_stanza_create "$leader" 90 || { + ko "$t" "no stanza-create ($mode)" + teardown_scope "$scope" + return + } + psql_leader "$leader" -c "SELECT pg_switch_wal();" >/dev/null + if ! wait_for_watcher_backup "$leader" full 120; then + ko "$t" "watcher did not take initial full within 120s ($mode)" + fail_dump "$t" "$leader" + teardown_scope "$scope" + return + fi + if [ "$mode" = heuristic ]; then + if ! docker logs "$leader" 2>&1 | grep -qF "$line"; then + ko "$t" "watcher did not switch to --start-fast for checkpoint_timeout=20min" + fail_dump "$t" "$leader" + teardown_scope "$scope" + return + fi + elif docker logs "$leader" 2>&1 | grep -qF "backup start: a spread checkpoint"; then + ko "$t" "watcher overrode an explicit PGBACKREST_START_FAST" + fail_dump "$t" "$leader" + teardown_scope "$scope" + return + fi + teardown_scope "$scope" + done + ok "$t" + note "--start-fast chosen for checkpoint_timeout=20min; explicit PGBACKREST_START_FAST left alone" +} + t_watcher_periodic_full() { local scope=t-period-full-${PG_VERSION} reset_bucket @@ -7181,6 +7242,7 @@ ALL_TESTS=( t_archiving_boot t_pitr_happy_path t_watcher_initial_full + t_backup_start_fast_when_checkpoint_outlasts_db_timeout t_watcher_periodic_full t_watcher_periodic_diff t_watcher_gap_recovery_full