diff --git a/AGENTS.md b/AGENTS.md index b8ef622..174b3cf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,7 +13,7 @@ These are the working rules for agents in this repo. pgcheckup is a read-only CL Keep commands cross-platform (`dotnet`, `docker`), because the owner develops on Windows. Avoid bash-only scripts. - Build: `dotnet build pgcheckup.slnx`. A broken check folder fails the build with its file and line. -- Test: `dotnet test --project tests/Pgcheckup.Tests`. It needs Docker, and uses Postgres 18 unless `PGCHECKUP_TEST_POSTGRES` names another major (14 to 17). CI runs all five. +- Test: `dotnet test --project tests/Pgcheckup.Tests`. It needs Docker, and uses Postgres 18 unless `PGCHECKUP_TEST_POSTGRES` names another major (14 to 17). CI runs all five. `PGCHECKUP_TEST_CHECK=` runs only that check's fixtures, which is quicker while writing a check. - Publish: `dotnet publish src/Pgcheckup -c Release -r win-x64 -o out` (`linux-x64` on Linux). Trim and AOT warnings fail it. - Test the published binary: set `PGCHECKUP_BINARY` to it, then run `dotnet test --project tests/Pgcheckup.Tests -- --filter-class Pgcheckup.Tests.Cli.NativeBinaryTests`. - NativeAOT publish on this Windows machine fails with `'vswhere.exe' is not recognized` unless the VS Installer folder is on PATH. Run it as `$env:PATH = "C:\Program Files (x86)\Microsoft Visual Studio\Installer;$env:PATH"; dotnet publish …`. That is an environment problem, not an AOT warning. @@ -24,7 +24,7 @@ The product is only as good as these rules. Never break them, not even in debug - **Read-only, always.** A check is one `SELECT` against catalogs and statistics views. No DDL or DML, and no functions with side effects: `pg_terminate_backend`, `pg_cancel_backend`, `pg_reload_conf`, `pg_stat_reset*`, `pg_switch_wal`, `pg_create_*`, `pg_drop_*`, `pg_advisory_*`, `nextval`, `setval`, `set_config`, `txid_current`. Every statement pgcheckup sends runs inside `BEGIN READ ONLY` with `SET LOCAL statement_timeout`, `lock_timeout` and `search_path = pg_catalog, pg_temp`, then rolls back. Never set anything for the whole session, because behind a transaction pooler it reaches the app's connections. Never weaken or bypass these guards. - **Fixes are text.** pgcheckup prints fix SQL and never executes it. -- **Least privilege.** No check needs more than `pg_monitor`. Never require superuser or `rds_superuser`. If the role lacks a privilege, the check is skipped with the reason. It is never an error. +- **Least privilege.** No check needs more than `pg_monitor`, except `integer-exhaustion`, which needs SELECT on sequences (counters only, never rows). Never require superuser or `rds_superuser`. If the role lacks a privilege, the check is skipped with the reason. It is never an error. - **No network beyond the Postgres connection.** No telemetry, update checks, crash reporting or remote lookups. Data such as end-of-life dates ships inside the release. - **No secrets or data in output.** Never print or log a password or a full connection string, query text (`pg_stat_activity.query`, `pg_stat_statements.query`), row data or client addresses. Findings name objects, settings, process IDs and durations only. - **Placeholders everywhere.** Docs, fixtures, tests and issues use `db.example.com`, `app` and `checkup`, never real hosts or credentials. @@ -36,10 +36,10 @@ The product is only as good as these rules. Never break them, not even in debug - `check.sql` is one read-only query that returns values, never prose. The wording lives in the `message` and `fix` templates in `check.md`. Thresholds come in as `@name` parameters and are never hard-coded. The search path is `pg_catalog` only, so qualify anything in another schema. - Compute ages and durations in SQL from the server's `now()`, not the client's clock. - `check.md` has **What breaks**, **Fix** and **Seen in** sections. Every check has at least one **Seen in** link to a public incident or the Postgres docs. Never cite anything a reader can't open. -- Both fixtures are required. `fires.sql` is the positive control, so a check without one isn't done. A fixture may lower a threshold (`-- threshold name = value`) when the real condition can't be reproduced at scale. +- Both fixtures are required. `fires.sql` is the positive control, so a check without one isn't done. A fixture may lower a threshold (`-- threshold name = value`) when the real condition can't be reproduced at scale, start Postgres with a setting that needs a restart (`-- server name = value`), and let its next statement fail (`-- expect error`). - Check ids are kebab-case and stable, because baselines and ignore lists depend on them. Renaming one is a breaking change that needs a decision in `ROADMAP.md`. - Severity: `critical` can take the database down or lose data soon. `warning` is heading there, or removes a safety net. `info` is housekeeping. Don't inflate severity. -- A check declares its minimum Postgres version and the providers where it is skipped. Every check is tested on every supported version. +- A check declares its minimum Postgres version, the privileges it needs and the providers where it is skipped. Every check is tested on every supported version, and its `fires` fixture is tested as a role with exactly its declared privileges. ## .NET and NativeAOT diff --git a/README.md b/README.md index 9c5969c..626fec6 100644 --- a/README.md +++ b/README.md @@ -11,9 +11,9 @@ In February 2019, one of the Postgres shards behind Mailchimp's Mandrill [ran ou Most of these failures show up in the system catalogs weeks ahead: a table's transaction ID age, a replication slot nobody reads, WAL archiving that failed last night. Teams without a DBA rarely look. pgcheckup looks for them and tells you what to do. -> **Status:** early development. The first check, `replication-slot-inactive`, runs end to end. There is no release to install yet. See [ROADMAP.md](ROADMAP.md). +> **Status:** early development. The 15 checks of v0.1 run end to end, but there is no release to install yet. See [ROADMAP.md](ROADMAP.md). -![pgcheckup scanning a database whose inactive replication slot is holding 1.07 GB of WAL](docs/scan.gif) +![pgcheckup scanning a database: an inactive replication slot holding 1.07 GB of WAL, no WAL limit for slots, and no timeout for idle transactions](docs/scan.gif) ## How it works @@ -26,24 +26,30 @@ pgcheckup scan "postgres://checkup@db.example.com:5432/app?sslmode=verify-full" pgcheckup · app on db.example.com · PostgreSQL 17.6 · Amazon RDS CRITICAL xid-wraparound - Table orders has used 1.61 billion of its 2.1 billion transaction IDs. - Vacuum can't freeze it while pid 4127 holds a transaction open (6 days). - Fix: end pid 4127, then run VACUUM (FREEZE) orders; + Table public.orders has used 1.61 billion of its 2.1 billion transaction IDs. + Fix: VACUUM (FREEZE, VERBOSE) public.orders; + +WARNING long-transaction + Session 4127 (worker on app) has had a transaction open for 6 days and has been idle in it for 6 days. + Fix: if the session is stuck or abandoned, end it: + SELECT pg_terminate_backend(4127); WARNING replication-slot-inactive Slot debezium has been inactive for 3 days and is holding 48 GB of WAL. Fix: restart its consumer, or drop the slot: SELECT pg_drop_replication_slot('debezium'); -11 passed · 1 critical · 1 warning · 1 skipped (wal-archiving-failing: managed by Amazon RDS) +11 passed · 1 critical · 2 warnings · 1 skipped (wal-archiving-failing: managed by Amazon RDS) ``` ## What it checks -- **Running out of IDs:** transaction ID and multixact wraparound, and `int4` sequences and identity columns near their limit. -- **Cleanup that can't run:** long transactions, sessions idle inside a transaction, forgotten prepared transactions, and tables with autovacuum turned off. -- **Disks filling with WAL:** inactive replication slots, slots with no WAL limit, and failing WAL archiving. -- **Capacity and settings:** connection saturation, `fsync`, `full_page_writes` or `autovacuum` turned off, invalid indexes, and Postgres versions past end of life. +- **Running out of IDs:** transaction ID and multixact wraparound, and `int4` sequences, identity columns and foreign keys near their limit. +- **Cleanup that can't run:** long transactions and sessions idle inside one, forgotten prepared transactions, no timeout for idle transactions, and autovacuum turned off. +- **Disks filling with WAL:** inactive replication slots, slots with no WAL limit, and WAL archiving that fails or hangs. +- **Capacity and settings:** connection saturation, `fsync` or `full_page_writes` turned off, invalid indexes, collations changed by an OS upgrade, and Postgres versions near or past end of life. + +`pgcheckup list` shows every check. Every finding says what breaks and how to fix it. `pgcheckup explain ` prints the full note, with links to incidents where it happened. @@ -57,7 +63,7 @@ Every finding says what breaks and how to fix it. `pgcheckup explain ` pr ## In CI -`pgcheckup scan` exits 0 when no finding reaches `--fail-on` (default `critical`), 1 when one does, and 2 when the scan couldn't run. `--format json` and `--format markdown` are there for pipelines and pull requests. A baseline, so CI fails only on new findings, comes with v0.2. +`pgcheckup scan` exits 1 when a finding reaches `--fail-on` (default `critical`), otherwise 2 when the scan couldn't run or a check errored, and 0 when neither happened. `--format json` ([its shape](docs/json.md)) and `--format markdown` are there for pipelines and pull requests. A baseline, so CI fails only on new findings, comes with v0.2. ## Prior art diff --git a/ROADMAP.md b/ROADMAP.md index 4f70603..618bc7d 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -7,7 +7,7 @@ pgcheckup is a read-only CLI (.NET 10, NativeAOT) that checks a PostgreSQL datab - **Name.** The idea started as "Postgres Doctor". `pgdoctor` is already an existing Go project, so this one is pgcheckup. PostgresAI's `postgres-checkup` is unrelated, and the README says so. - **Position.** Production readiness for teams without a DBA. It has fewer checks than pg-healthcheck or pgdoctor, and each one maps to a failure that causes outages and is explained in plain words with its fix. It is not a DBA toolkit. - **Read-only by construction.** Every statement pgcheckup sends runs in its own transaction: `BEGIN READ ONLY`, `SET LOCAL statement_timeout = '5s'`, `SET LOCAL lock_timeout = '1s'`, `SET LOCAL search_path = pg_catalog, pg_temp`, the query, then `ROLLBACK`. The pinned search path stops a function planted in another schema from shadowing a built-in and running as the scanning role. Only `application_name = pgcheckup` is set for the whole session. Behind a transaction-mode pooler such as PgBouncer, a session-level `SET` would reach the app's next transaction, and the `options` connection parameter is rejected or dropped. The lock timeout stops a scan from queueing behind a migration's lock and then blocking the traffic behind it. pgcheckup prints fixes and never runs them. -- **Least privilege.** No check needs more than `pg_monitor`. Each check declares what it needs, and if the role doesn't have it, the check is skipped with the reason. `pgcheckup grant` prints the SQL for a checkup role, including `ALTER ROLE … SET default_transaction_read_only = on`, so the role is read-only outside pgcheckup too. +- **Least privilege.** No check needs more than `pg_monitor`, with one exception decided on 2026-09-26: `integer-exhaustion` (below). Each check declares what it needs, and if the role doesn't have it, the check is skipped with the reason. `pgcheckup grant` prints the SQL for a checkup role, including `ALTER ROLE … SET default_transaction_read_only = on`, so the role is read-only outside pgcheckup too. - **SQL only.** pgcheckup talks only to Postgres. Provider settings that SQL can see (such as `rds.force_ssl`) are in scope. Checks that need a cloud API (RDS backups, deletion protection, encryption at rest) are not. - **Managed providers.** pgcheckup detects RDS/Aurora, Cloud SQL, Azure Database for PostgreSQL, Supabase and Neon from SQL. A check can list providers where it doesn't apply or can't run, and it shows as skipped there, with the reason. - **Checks are SQL plus Markdown**, one folder per check under `checks//`: @@ -21,7 +21,7 @@ pgcheckup is a read-only CLI (.NET 10, NativeAOT) that checks a PostgreSQL datab - **Postgres versions:** every community-supported major (14 to 18 today), each tested in CI with Testcontainers. When a major reaches end of life, it moves to best effort, and scanning it reports the end of life as a finding. - **Fixture tests** give each fixture a fresh Testcontainers Postgres, because slots, prepared transactions and roles belong to the whole server. The fixture's connection stays open until the check has run, so a fixture can hold a transaction open. The check runs through the same code as `scan`, as a role with only `pg_monitor`. - **Output:** terminal (default), `--format json` and `--format markdown` in v0.1. The JSON has a `schema` version. The HTML report comes in v0.2. -- **Exit codes:** 0 when no finding reaches `--fail-on` (default `critical`), 1 when one does, and 2 when the scan couldn't run. Skipped checks never fail a scan. +- **Exit codes:** 1 when a finding reaches `--fail-on` (default `critical`). Otherwise 2 when the scan couldn't run or any check errored (errors added 2026-09-26), and 0 when neither happened. The report always prints in full. A finding outranks an error because it is the more useful signal, and an error still fails CI. Skipped checks never fail a scan. - **Severity:** `critical` can take the database down or lose data soon. `warning` is heading there, or removes a safety net. `info` is housekeeping. - **Connection:** a `postgres://` URL, a key-value connection string, or the standard `PG*` environment variables and `.pgpass`. Docs keep passwords off the command line. - **What output may contain.** Findings name database objects (tables, slots, roles), settings, process IDs and durations. They never include query text, row data, passwords or client addresses. @@ -32,6 +32,17 @@ pgcheckup is a read-only CLI (.NET 10, NativeAOT) that checks a PostgreSQL datab - **Brand** is option 1A, "Scan": stacked layers read by a single probe line. The wordmark is Bricolage Grotesque SemiBold (optical size 34), converted to vector paths, with "pg" in Postgres blue (`#336791`, or `#5B9BD5` on dark). The assets are in `docs/brand/`, with `-dark` files for dark backgrounds. - **License:** Apache-2.0. +## Decisions (2026-09-26) + +- **`integer-exhaustion` reads sequence counters,** which `pg_monitor` can't see: `pg_sequences.last_value` is NULL and `pg_sequence_last_value()` is denied. The check needs SELECT on sequences, which shows counters but no table rows. It is the one exception to the `pg_monitor` ceiling. Without the grant it is skipped with the reason, and `pgcheckup grant` prints the extra lines. +- **Provider detection** reads roles and settings: `rds_superuser` for RDS, the `aurora_version()` function for Aurora, `cloudsqlsuperuser` for Cloud SQL, `azure_pg_admin` for Azure, `supabase_admin` for Supabase, and `neon.*` settings for Neon. Fixtures create them to test each one. +- **Declared privileges are tested.** Each check's `fires` fixture must still fire when run as a role with exactly the privileges the check declares. Without `pg_read_all_stats`, for example, `pg_stat_activity` hides other users' sessions, so a check could pass while seeing nothing. +- **Fixture directives.** `-- server name = value` starts the fixture's container with that setting, for settings that need a restart (`max_prepared_transactions`, `archive_mode`). `-- expect error` lets the next statement fail, such as a `CREATE INDEX CONCURRENTLY` that leaves an invalid index. +- **`postgres-eol` is a SQL check.** The end-of-life dates ship in a `VALUES` list in its `check.sql` and are compared with the server's clock, so no client clock is read. +- **Quiet on stock Postgres.** Stock Postgres ships with `max_slot_wal_keep_size = -1` and no `idle_in_transaction_session_timeout`, and flagging every database for them would teach people to ignore pgcheckup. `replication-slot-unbounded` fires only when a slot exists. An unset timeout is `info`, and sessions idle in a transaction are `warning`. +- **The check table follows the desk research.** Public postmortems cluster around vacuum: transaction ID wraparound (6) and old snapshots holding vacuum back (5), then WAL filling the disk and connection exhaustion (3 each). So `collation-version-mismatch` joins, sessions idle in a transaction are reported by `long-transaction`, `idle-in-transaction` becomes `idle-transaction-timeout`, and the autovacuum settings move from `dangerous-settings` into `autovacuum-disabled` (was `autovacuum-disabled-table`). Lock queues, missing statistics, MultiXact member space and pooler exhaustion can't be predicted from catalog state. `sync-standby-missing` has too little evidence for v0.1 and goes under Later. +- **JSON `schema: 1`** holds the server, a summary, and each check's status (passed, critical, warning, info, skipped or errored) with its reason and findings. A finding has a subject, severity, message, fix and values. The values are the columns its message uses, in raw form: bytes as integers, durations in seconds, and timestamps in ISO 8601 UTC. `docs/json.md` documents the shape. + ## M0: Placeholder (as soon as possible) - [x] Add `LICENSE` (Apache-2.0). @@ -48,31 +59,32 @@ pgcheckup is a read-only CLI (.NET 10, NativeAOT) that checks a PostgreSQL datab ## M1: Engine and check catalog -- [ ] Engine: server version and provider detection, and a privilege probe, followed by every applicable check. A check that errors or times out is reported as errored, and the others still run. -- [ ] Provider detection, tested by simulating each provider's roles and settings in fixtures. -- [ ] Desk research for the catalog: go through public Postgres postmortems (danluu/post-mortems, engineering blogs) and DBA Stack Exchange, list the failures that recur, and adjust the table below to match. Every check gets at least one **Seen in** link. -- [ ] The v0.1 checks: +- [x] Engine: server version and provider detection, and a privilege probe, followed by every applicable check. A check that errors or times out is reported as errored, and the others still run. +- [x] Provider detection, tested by simulating each provider's roles and settings in fixtures. +- [x] Desk research for the catalog: go through public Postgres postmortems (danluu/post-mortems, engineering blogs) and DBA Stack Exchange, list the failures that recur, and adjust the table below to match. Every check gets at least one **Seen in** link. +- [x] The v0.1 checks: | Check | Catches | |---|---| | `xid-wraparound` | Databases and tables whose oldest unfrozen transaction ID is nearing the 2.1 billion limit | | `multixact-wraparound` | The same for multixact IDs | -| `long-transaction` | Transactions open longer than the threshold, which hold back vacuum | -| `idle-in-transaction` | Sessions idle inside an open transaction, and `idle_in_transaction_session_timeout` left unset | +| `long-transaction` | Transactions open, or idle, longer than the threshold, which hold back vacuum | +| `idle-transaction-timeout` | `idle_in_transaction_session_timeout` left unset, so an abandoned transaction stays open until someone ends it | | `prepared-transaction-orphaned` | Prepared transactions left behind, which hold locks and block vacuum | -| `replication-slot-inactive` | Slots with no consumer, which keep WAL until the disk fills | -| `replication-slot-unbounded` | `max_slot_wal_keep_size = -1`, so one stuck slot can keep unlimited WAL | -| `wal-archiving-failing` | `archive_command` failing since the last success, which breaks point-in-time recovery | +| `replication-slot-inactive` | Slots with no consumer, which keep WAL until the disk fills, or pin `xmin` and block vacuum | +| `replication-slot-unbounded` | Slots with no cap on the WAL they keep, so one stuck slot can fill the disk | +| `wal-archiving-failing` | WAL archiving that fails, hangs or has no command, which breaks point-in-time recovery and keeps WAL | | `connection-saturation` | Connections close to `max_connections` minus the reserved slots | -| `dangerous-settings` | `fsync`, `full_page_writes` or `autovacuum` turned off | -| `autovacuum-disabled-table` | Tables with `autovacuum_enabled = false` | -| `integer-exhaustion` | `int4` sequences and identity columns past a share of their range | +| `dangerous-settings` | `fsync` or `full_page_writes` turned off, or `zero_damaged_pages` turned on | +| `autovacuum-disabled` | `autovacuum` or `track_counts` turned off, and tables with `autovacuum_enabled = false` | +| `integer-exhaustion` | `int4` sequences, identity columns and foreign keys past a share of their range | | `invalid-index` | Indexes left invalid by a failed `CREATE INDEX CONCURRENTLY`, which slow writes and are never used | -| `postgres-eol` | Major versions past their end-of-life date | +| `collation-version-mismatch` | Databases and collations whose version changed under them after an OS upgrade, which breaks text indexes | +| `postgres-eol` | Major versions past, or close to, their end-of-life date | -- [ ] `pgcheckup explain ` prints the check's note. `pgcheckup list` shows every check with its category and minimum version. -- [ ] `pgcheckup grant` prints SQL for a least-privilege checkup role: `pg_monitor`, `CONNECT`, and `default_transaction_read_only = on` for the role. -- [ ] `--format json` and `--format markdown`. The JSON shape is documented, with `"schema": 1`. +- [x] `pgcheckup explain ` prints the check's note. `pgcheckup list` shows every check with its category and minimum version. +- [x] `pgcheckup grant` prints SQL for a least-privilege checkup role: `pg_monitor`, `CONNECT`, `default_transaction_read_only = on` for the role, and SELECT on sequences for `integer-exhaustion`. +- [x] `--format json` and `--format markdown`. The JSON shape is documented, with `"schema": 1`. **Done when:** every check's fixtures pass on Postgres 14 to 18, and a scan as a role with only `pg_monitor` either runs or skips (with a reason) every check, with no errors. @@ -81,6 +93,7 @@ pgcheckup is a read-only CLI (.NET 10, NativeAOT) that checks a PostgreSQL datab - [ ] Release workflow: NativeAOT binaries for linux-x64, linux-arm64, osx-arm64 and win-x64 on GitHub Releases, with SHA-256 checksums. - [ ] Container image on GHCR (amd64, arm64), and a GitHub Actions example in the README. - [ ] Error messages: connection, TLS, authentication and missing-privilege failures each say what to do next. +- [ ] Postgres 14 reaches end of life on 2026-11-12. Move it to best effort: drop it from the CI matrix, where `postgres-eol`'s healthy fixture starts failing on it that day. - [ ] Dogfooding log: scan real databases, including at least one managed provider, and record every false alarm and every missed problem. - [ ] Launch gates: README quick start tested on a clean machine, supported platforms and Postgres versions documented, `CONTRIBUTING.md` with build and test steps, `SECURITY.md`, no known critical bugs, and green CI. @@ -103,6 +116,7 @@ pgcheckup is a read-only CLI (.NET 10, NativeAOT) that checks a PostgreSQL datab - Hosted monitoring (scheduled scans with alerts), then a team dashboard - Scanning every database in a cluster in one run - Checks based on `pg_stat_statements` +- `sync-standby-missing`: `synchronous_standby_names` is set, but no synchronous standby is connected, so every commit hangs - Scoop, Homebrew and winget packages - Signed releases with build provenance diff --git a/checks/autovacuum-disabled/check.md b/checks/autovacuum-disabled/check.md new file mode 100644 index 0000000..0324717 --- /dev/null +++ b/checks/autovacuum-disabled/check.md @@ -0,0 +1,40 @@ +--- +id: autovacuum-disabled +title: Autovacuum turned off +category: cleanup +severity: warning +min_version: 14 +privileges: [] +message: "[{setting} is off, so dead rows pile up in every table, and tables are only frozen when wraparound forces it.][Table {table_name} has autovacuum turned off, so its dead rows pile up, and it is only frozen when wraparound forces it.]" +fix: "[ALTER SYSTEM SET {setting} = on;\nSELECT pg_reload_conf();][ALTER TABLE {table_name} RESET (autovacuum_enabled, toast.autovacuum_enabled);]" +--- + +## What breaks + +Autovacuum removes the dead rows that updates and deletes leave behind, keeps planner statistics current, and freezes old rows before transaction IDs wrap around. With it off, tables and indexes bloat, queries slow down as statistics go stale, and freezing only happens in the emergency anti-wraparound vacuum, which can't be cancelled and blocks schema changes. + +Autovacuum needs `track_counts` to know which tables changed, so turning that off stops it too. A single table can also have it turned off with `autovacuum_enabled = false`, often left over from a bulk load. + +The check looks at the server's settings, and at the tables of the database it connects to. + +## Fix + +Turn it back on for the server: + +```sql +ALTER SYSTEM SET autovacuum = on; +SELECT pg_reload_conf(); +``` + +Or for a table: + +```sql +ALTER TABLE public.events RESET (autovacuum_enabled, toast.autovacuum_enabled); +``` + +If a table was off for long, run `VACUUM (ANALYZE, VERBOSE)` on it once. + +## Seen in + +- [A production story: downtime caused by Postgres transaction ID wraparound](https://www.sqlservercentral.com/articles/i-too-have-a-production-story-a-downtime-caused-by-postgres-transaction-id-wraparound-problem), SQLServerCentral +- [autovacuum_enabled storage parameter](https://www.postgresql.org/docs/current/sql-createtable.html#RELOPTION-AUTOVACUUM-ENABLED), PostgreSQL documentation diff --git a/checks/autovacuum-disabled/check.sql b/checks/autovacuum-disabled/check.sql new file mode 100644 index 0000000..e33c9d4 --- /dev/null +++ b/checks/autovacuum-disabled/check.sql @@ -0,0 +1,19 @@ +SELECT s.name AS subject, + s.name AS setting, + NULL::text AS table_name +FROM pg_settings AS s +WHERE s.name IN ('autovacuum', 'track_counts') AND s.setting = 'off' +UNION ALL +-- Tables in this database. The option is stored as written, in any spelling Postgres accepts +-- for false, and a TOAST table carries its own. +SELECT format('%I.%I', n.nspname, c.relname), + NULL, + format('%I.%I', n.nspname, c.relname) +FROM pg_class AS c +JOIN pg_namespace AS n ON n.oid = c.relnamespace +LEFT JOIN pg_class AS t ON t.oid = c.reltoastrelid +WHERE c.relkind IN ('r', 'm') + AND EXISTS ( + SELECT FROM unnest(c.reloptions || coalesce(t.reloptions, '{}')) AS o(option) + WHERE o.option ~* '^autovacuum_enabled=(f(a(l(se?)?)?)?|off?|no?|0)$' + ) diff --git a/checks/autovacuum-disabled/fixtures/fires.sql b/checks/autovacuum-disabled/fixtures/fires.sql new file mode 100644 index 0000000..4400a82 --- /dev/null +++ b/checks/autovacuum-disabled/fixtures/fires.sql @@ -0,0 +1,3 @@ +-- server autovacuum = off +-- Off for the server, and for a table left over from a bulk load. +CREATE TABLE fixture_events (id int) WITH (autovacuum_enabled = off); diff --git a/checks/autovacuum-disabled/fixtures/healthy.sql b/checks/autovacuum-disabled/fixtures/healthy.sql new file mode 100644 index 0000000..54b659c --- /dev/null +++ b/checks/autovacuum-disabled/fixtures/healthy.sql @@ -0,0 +1,2 @@ +-- Set explicitly on, which the check must not mistake for off. +CREATE TABLE fixture_events (id int) WITH (autovacuum_enabled = on); diff --git a/checks/collation-version-mismatch/check.md b/checks/collation-version-mismatch/check.md new file mode 100644 index 0000000..f5b489b --- /dev/null +++ b/checks/collation-version-mismatch/check.md @@ -0,0 +1,35 @@ +--- +id: collation-version-mismatch +title: Collation changed under the database +category: capacity +severity: critical +min_version: 15 +privileges: [] +message: "[Database {database}][Collation {collation_name}] was recorded with version {recorded}, but the system now provides {actual}. Text indexes that use it can return wrong results or let duplicate keys in." +fix: "[rebuild its indexes while connected to it, then record the new version:\nREINDEX DATABASE {database_ident};\nALTER DATABASE {database_ident} REFRESH COLLATION VERSION;][rebuild the indexes that use it, then record the new version:\nALTER COLLATION {collation_name} REFRESH VERSION;]" +--- + +## What breaks + +Text indexes are sorted by a collation, which comes from the operating system's C library (glibc) or from ICU. When an OS upgrade changes how that library sorts, as glibc 2.28 did for many languages, the order stored in existing indexes no longer matches. Lookups miss rows that are there, and unique indexes let duplicates in. Nothing reports an error; the data just goes quietly wrong. + +Postgres records each collation's version when it is created, and from Postgres 15 the version of each database's default collation too. This check compares those with what the system provides now. It looks at every database's default collation, and at collations that indexes use in the database it connects to. + +When the system can't report a version (the C collation, or a C library that doesn't version its collations), there is nothing to compare, and the check stays quiet. + +## Fix + +Rebuild the affected indexes, then record the new version so Postgres stops warning: + +```sql +REINDEX DATABASE app; +ALTER DATABASE app REFRESH COLLATION VERSION; +``` + +`REINDEX DATABASE` rebuilds every index and takes locks as it goes. On a large database, rebuild only the text indexes, with `REINDEX INDEX CONCURRENTLY`. A unique index that now holds duplicates fails to rebuild until you remove them. + +## Seen in + +- [Unique constraint violation in Postgres due to an OS upgrade](https://support.atlassian.com/bitbucket-data-center/kb/unique-constraint-violation-in-postgres-due-to-os-upgrade/), Atlassian +- [Upgrading the operating system for PostgreSQL](https://docs.gitlab.com/administration/postgresql/upgrading_os/), GitLab +- [glibc collations and data corruption](https://www.crunchydata.com/blog/glibc-collations-and-data-corruption), Crunchy Data diff --git a/checks/collation-version-mismatch/check.sql b/checks/collation-version-mismatch/check.sql new file mode 100644 index 0000000..8943267 --- /dev/null +++ b/checks/collation-version-mismatch/check.sql @@ -0,0 +1,30 @@ +-- Each database's default collation, recorded from Postgres 15 on. +SELECT d.datname AS subject, + d.datname AS database, + quote_ident(d.datname) AS database_ident, + NULL::text AS collation_name, + d.datcollversion AS recorded, + v.actual +FROM pg_database AS d +CROSS JOIN LATERAL (SELECT pg_database_collation_actual_version(d.oid) AS actual) AS v +WHERE d.datallowconn + AND d.datcollversion IS NOT NULL + AND v.actual IS NOT NULL + AND v.actual <> d.datcollversion +UNION ALL +-- Collations that indexes use in this database. Checking only those avoids asking ICU for the +-- version of each of the hundreds of collations Postgres imports. +SELECT format('%I.%I', n.nspname, c.collname), + NULL, + NULL, + format('%I.%I', n.nspname, c.collname), + c.collversion, + v.actual +FROM pg_collation AS c +JOIN pg_namespace AS n ON n.oid = c.collnamespace +CROSS JOIN LATERAL (SELECT pg_collation_actual_version(c.oid) AS actual) AS v +WHERE c.oid IN (SELECT unnest(x.indcollation::oid[]) FROM pg_index AS x) + AND c.collversion IS NOT NULL + AND v.actual IS NOT NULL + AND v.actual <> c.collversion +ORDER BY 1 diff --git a/checks/collation-version-mismatch/fixtures/fires.sql b/checks/collation-version-mismatch/fixtures/fires.sql new file mode 100644 index 0000000..f907043 --- /dev/null +++ b/checks/collation-version-mismatch/fixtures/fires.sql @@ -0,0 +1,8 @@ +-- An indexed column with an ICU collation, then the version Postgres recorded is set back, as +-- if the library had been upgraded since. +CREATE COLLATION fixture_collation (provider = icu, locale = 'en-US'); +CREATE TABLE fixture_names (name text COLLATE fixture_collation); +CREATE INDEX fixture_names_name ON fixture_names (name); +UPDATE pg_collation SET collversion = '1.0' WHERE collname = 'fixture_collation'; +-- The same for the database's default collation, from glibc. +UPDATE pg_database SET datcollversion = '1.0' WHERE datname = 'app'; diff --git a/checks/collation-version-mismatch/fixtures/healthy.sql b/checks/collation-version-mismatch/fixtures/healthy.sql new file mode 100644 index 0000000..329ec89 --- /dev/null +++ b/checks/collation-version-mismatch/fixtures/healthy.sql @@ -0,0 +1,3 @@ +CREATE COLLATION fixture_collation (provider = icu, locale = 'en-US'); +CREATE TABLE fixture_names (name text COLLATE fixture_collation); +CREATE INDEX fixture_names_name ON fixture_names (name); diff --git a/checks/connection-saturation/check.md b/checks/connection-saturation/check.md new file mode 100644 index 0000000..8c05088 --- /dev/null +++ b/checks/connection-saturation/check.md @@ -0,0 +1,38 @@ +--- +id: connection-saturation +title: Connections running out +category: capacity +severity: warning +min_version: 14 +privileges: [pg_read_all_stats] +thresholds: + warning_ratio: 0.8 + critical_ratio: 0.95 +message: "{used:count} of the {available:count} connections available to applications are in use. When they run out, Postgres refuses new connections." +fix: | + see who holds them: + SELECT usename, state, count(*) FROM pg_stat_activity GROUP BY 1, 2 ORDER BY 3 DESC; + -- then close idle sessions, shrink the pools, or put a pooler such as PgBouncer in front. +--- + +## What breaks + +Postgres accepts at most `max_connections` sessions, and keeps a few of them back for superusers (`superuser_reserved_connections`, and `reserved_connections` from Postgres 16). When applications have used the rest, every new connection fails with "too many clients already". Deploys, cron jobs and autoscaling are the usual moments: each new process opens its own pool. + +Raising `max_connections` helps less than it seems, because each connection costs memory and the server slows down with too many active at once. A pooler such as PgBouncer lets many clients share a few connections. + +## Fix + +Find out which roles and states hold the connections: + +```sql +SELECT usename, state, count(*) FROM pg_stat_activity GROUP BY 1, 2 ORDER BY 3 DESC; +``` + +Many `idle` sessions point to pools that are too large for the number of processes. Put a pooler in front, or shrink each pool. + +## Seen in + +- [Database outage: too many connections](https://gitlab.com/gitlab-com/gl-infra/production/-/issues/122), GitLab +- [PostgreSQL connections exhausted](https://gitlab.com/gitlab-org/omnibus-gitlab/-/issues/8292), GitLab +- [How to increase the max connections in Postgres](https://dba.stackexchange.com/questions/69438), DBA Stack Exchange diff --git a/checks/connection-saturation/check.sql b/checks/connection-saturation/check.sql new file mode 100644 index 0000000..10ab699 --- /dev/null +++ b/checks/connection-saturation/check.sql @@ -0,0 +1,15 @@ +WITH available AS ( + -- reserved_connections arrived in Postgres 16. + SELECT current_setting('max_connections')::int + - current_setting('superuser_reserved_connections')::int + - coalesce(current_setting('reserved_connections', true)::int, 0) AS slots +), +used AS ( + SELECT count(*) AS sessions FROM pg_stat_activity WHERE backend_type = 'client backend' +) +SELECT 'max_connections' AS subject, + CASE WHEN u.sessions >= a.slots * @critical_ratio THEN 'critical' END AS severity, + u.sessions AS used, + a.slots AS available +FROM available AS a, used AS u +WHERE u.sessions >= a.slots * @warning_ratio diff --git a/checks/connection-saturation/fixtures/fires.sql b/checks/connection-saturation/fixtures/fires.sql new file mode 100644 index 0000000..a0522c1 --- /dev/null +++ b/checks/connection-saturation/fixtures/fires.sql @@ -0,0 +1,5 @@ +-- server max_connections = 6 +-- threshold warning_ratio = 0.6 +-- Three slots are left for applications. This session and the check's make two, which only +-- reaches the threshold if the check can see this session. +SELECT 1; diff --git a/checks/connection-saturation/fixtures/healthy.sql b/checks/connection-saturation/fixtures/healthy.sql new file mode 100644 index 0000000..1117d7c --- /dev/null +++ b/checks/connection-saturation/fixtures/healthy.sql @@ -0,0 +1,2 @@ +-- A handful of sessions on stock Postgres, which allows 100. +SELECT 1; diff --git a/checks/dangerous-settings/check.md b/checks/dangerous-settings/check.md new file mode 100644 index 0000000..0f43bb6 --- /dev/null +++ b/checks/dangerous-settings/check.md @@ -0,0 +1,43 @@ +--- +id: dangerous-settings +title: Settings that risk data +category: capacity +severity: critical +min_version: 14 +privileges: [] +skip_on: [neon] +message: "{subject} is {setting}. Postgres then can't protect your data from a crash or a damaged page." +fix: | + turn it back: + ALTER SYSTEM SET {subject} = {safe_value}; + SELECT pg_reload_conf(); +--- + +## What breaks + +Three settings trade Postgres's protection of your data for speed or convenience: + +- `fsync = off`: Postgres doesn't wait for writes to reach the disk. After a crash or power loss, the database can be corrupt, not just missing recent commits. +- `full_page_writes = off`: after a crash, a page that was half written can't be repaired from WAL, which corrupts it. +- `zero_damaged_pages = on`: Postgres silently replaces damaged pages with empty ones, which destroys the rows in them. It is meant for a one-off rescue, never for normal running. + +They are sometimes turned off to speed up a bulk load or a test server, and left that way. + +Neon runs with `fsync = off` by design, because its storage layer makes writes durable, so the check is skipped there. + +## Fix + +Turn the setting back and reload: + +```sql +ALTER SYSTEM SET fsync = on; +SELECT pg_reload_conf(); +``` + +If the server ran with `fsync` or `full_page_writes` off through a crash, check it for corruption, for example with `pg_amcheck`. + +## Seen in + +- [fsync](https://www.postgresql.org/docs/current/runtime-config-wal.html#GUC-FSYNC), PostgreSQL documentation +- [full_page_writes](https://www.postgresql.org/docs/current/runtime-config-wal.html#GUC-FULL-PAGE-WRITES), PostgreSQL documentation +- [zero_damaged_pages](https://www.postgresql.org/docs/current/runtime-config-developer.html#GUC-ZERO-DAMAGED-PAGES), PostgreSQL documentation diff --git a/checks/dangerous-settings/check.sql b/checks/dangerous-settings/check.sql new file mode 100644 index 0000000..dc0857e --- /dev/null +++ b/checks/dangerous-settings/check.sql @@ -0,0 +1,7 @@ +SELECT s.name AS subject, + s.setting, + CASE s.name WHEN 'zero_damaged_pages' THEN 'off' ELSE 'on' END AS safe_value +FROM pg_settings AS s +WHERE (s.name IN ('fsync', 'full_page_writes') AND s.setting = 'off') + OR (s.name = 'zero_damaged_pages' AND s.setting = 'on') +ORDER BY s.name diff --git a/checks/dangerous-settings/fixtures/fires.sql b/checks/dangerous-settings/fixtures/fires.sql new file mode 100644 index 0000000..bad9ac8 --- /dev/null +++ b/checks/dangerous-settings/fixtures/fires.sql @@ -0,0 +1,2 @@ +-- server fsync = off +SELECT 1; diff --git a/checks/dangerous-settings/fixtures/healthy.sql b/checks/dangerous-settings/fixtures/healthy.sql new file mode 100644 index 0000000..9bbbcfe --- /dev/null +++ b/checks/dangerous-settings/fixtures/healthy.sql @@ -0,0 +1,2 @@ +-- Stock Postgres keeps all three safe. +SELECT 1; diff --git a/checks/idle-transaction-timeout/check.md b/checks/idle-transaction-timeout/check.md new file mode 100644 index 0000000..67b4306 --- /dev/null +++ b/checks/idle-transaction-timeout/check.md @@ -0,0 +1,39 @@ +--- +id: idle-transaction-timeout +title: No timeout for idle transactions +category: cleanup +severity: info +min_version: 14 +privileges: [] +message: "{subject} is not set, so a session left idle in a transaction holds back vacuum until someone ends it." +fix: | + set it for the whole server, or only for your application's role: + ALTER SYSTEM SET idle_in_transaction_session_timeout = '10min'; + SELECT pg_reload_conf(); +--- + +## What breaks + +Nothing yet. But without `idle_in_transaction_session_timeout`, a session that opens a transaction and then waits, because of an application bug, a crashed worker or a console left open, keeps it open indefinitely. While it does, vacuum can't clean up after any transaction since, and its locks stay held. `long-transaction` reports such sessions when they happen; this timeout ends them on its own. + +Postgres ships with the timeout off, so most databases get this finding. That is why it is `info`. + +The check passes when the timeout, or `transaction_timeout` on Postgres 17 and later, is set for the server, a database or a role. + +## Fix + +Pick a limit longer than any transaction your application means to leave idle, and set it for the server or for the application's role: + +```sql +ALTER SYSTEM SET idle_in_transaction_session_timeout = '10min'; +SELECT pg_reload_conf(); + +ALTER ROLE app SET idle_in_transaction_session_timeout = '10min'; +``` + +On a managed provider, set it in the parameter group or its equivalent. + +## Seen in + +- [A production story: downtime caused by Postgres transaction ID wraparound](https://www.sqlservercentral.com/articles/i-too-have-a-production-story-a-downtime-caused-by-postgres-transaction-id-wraparound-problem), SQLServerCentral +- [idle_in_transaction_session_timeout](https://www.postgresql.org/docs/current/runtime-config-client.html#GUC-IDLE-IN-TRANSACTION-SESSION-TIMEOUT), PostgreSQL documentation diff --git a/checks/idle-transaction-timeout/check.sql b/checks/idle-transaction-timeout/check.sql new file mode 100644 index 0000000..5a2c1a6 --- /dev/null +++ b/checks/idle-transaction-timeout/check.sql @@ -0,0 +1,11 @@ +-- The session's own value covers the server and anything set for pgcheckup's role and database; +-- pg_db_role_setting covers what is set for the application's roles and databases. +SELECT 'idle_in_transaction_session_timeout' AS subject +WHERE current_setting('idle_in_transaction_session_timeout') = '0' + -- transaction_timeout arrived in Postgres 17, and ends idle transactions too. + AND coalesce(current_setting('transaction_timeout', true), '0') = '0' + AND NOT EXISTS ( + SELECT FROM pg_db_role_setting AS s, unnest(s.setconfig) AS c(setting) + WHERE c.setting ~ '^(idle_in_transaction_session_timeout|transaction_timeout)=' + AND c.setting !~ '=0$' + ) diff --git a/checks/idle-transaction-timeout/fixtures/fires.sql b/checks/idle-transaction-timeout/fixtures/fires.sql new file mode 100644 index 0000000..c20e9a3 --- /dev/null +++ b/checks/idle-transaction-timeout/fixtures/fires.sql @@ -0,0 +1,2 @@ +-- Stock Postgres leaves the timeout off. +SELECT 1; diff --git a/checks/idle-transaction-timeout/fixtures/healthy.sql b/checks/idle-transaction-timeout/fixtures/healthy.sql new file mode 100644 index 0000000..d5c2059 --- /dev/null +++ b/checks/idle-transaction-timeout/fixtures/healthy.sql @@ -0,0 +1,2 @@ +-- Set for the database, it applies to every new session there. +ALTER DATABASE app SET idle_in_transaction_session_timeout = '10min'; diff --git a/checks/integer-exhaustion/check.md b/checks/integer-exhaustion/check.md new file mode 100644 index 0000000..4b5da86 --- /dev/null +++ b/checks/integer-exhaustion/check.md @@ -0,0 +1,43 @@ +--- +id: integer-exhaustion +title: Integer IDs running out +category: ids +severity: warning +min_version: 14 +privileges: [select_on_sequences] +thresholds: + warning_ratio: 0.5 + critical_ratio: 0.8 +message: "[Column {column_name} has used {used:count} of the {capacity:count} values it can hold.][Sequence {sequence_name} has used {used:count} of the {capacity:count} values it can hold.][Column {fk_column} holds values up to {capacity:count}, but it is a foreign key to {referenced}, which has reached {used:count}.]" +fix: "[move it to bigint. This rewrites the table under an exclusive lock, so plan it:\nALTER TABLE {table_name} ALTER COLUMN {attribute} TYPE bigint;][\nALTER SEQUENCE {owned_sequence} AS bigint;][ALTER SEQUENCE {sequence_name} AS bigint;]" +--- + +## What breaks + +An `integer` column holds values up to about 2.1 billion (`smallint`, 32,767). When the sequence that feeds it reaches that, every insert fails with "integer out of range" or "nextval: reached maximum value", and the application stops taking new rows. + +Three shapes lead there: + +- A `serial` or identity column declared `integer`, often from before anyone expected the table to grow. +- A sequence declared `AS integer` on its own, or left `integer` when its column was changed to `bigint`. + +Sequences with `CYCLE` start over by design, so the check ignores them. +- An `integer` foreign key that points at a `bigint` key. The key is fine, but once its values pass 2.1 billion, no row can reference them. + +Reading sequence counters needs SELECT on the sequences, which shows counters but no table rows. `pgcheckup grant` prints that grant. Without it, the check is skipped. + +## Fix + +Change the column to `bigint`, and its sequence with it: + +```sql +ALTER TABLE public.orders ALTER COLUMN id TYPE bigint; +ALTER SEQUENCE public.orders_id_seq AS bigint; +``` + +Changing a column's type rewrites the table and blocks it for the whole time, so plan it: on a large table, add a new `bigint` column, backfill it in batches, and swap it in. + +## Seen in + +- [Incident 2558: Heroku API unavailable](https://status.heroku.com/incidents/2558), Heroku +- [Sequence manipulation functions](https://www.postgresql.org/docs/current/functions-sequence.html), PostgreSQL documentation diff --git a/checks/integer-exhaustion/check.sql b/checks/integer-exhaustion/check.sql new file mode 100644 index 0000000..e493a5c --- /dev/null +++ b/checks/integer-exhaustion/check.sql @@ -0,0 +1,86 @@ +WITH sequences AS ( + SELECT s.seqrelid, + format('%I.%I', n.nspname, c.relname) AS sequence_name, + s.seqmax, + pg_sequence_last_value(s.seqrelid) AS last_value + FROM pg_sequence AS s + JOIN pg_class AS c ON c.oid = s.seqrelid + JOIN pg_namespace AS n ON n.oid = c.relnamespace + WHERE s.seqincrement > 0 + -- A cycling sequence starts over at its minimum by design. + AND NOT s.seqcycle + -- Another session's temporary sequence can't be read. + AND c.relpersistence <> 't' +), +feeds AS ( + -- The column a sequence feeds: 'a' for serial, 'i' for identity. + SELECT d.objid AS seqrelid, d.refobjid AS table_oid, d.refobjsubid AS attnum + FROM pg_depend AS d + WHERE d.classid = 'pg_class'::regclass + AND d.refclassid = 'pg_class'::regclass + AND d.deptype IN ('a', 'i') + AND d.refobjsubid > 0 +), +columns AS ( + SELECT a.attrelid, + a.attnum, + format('%I.%I', n.nspname, c.relname) AS table_name, + quote_ident(a.attname) AS attribute, + format('%I.%I.%I', n.nspname, c.relname, a.attname) AS column_name, + CASE a.atttypid + WHEN 'int2'::regtype THEN 32767 + WHEN 'int4'::regtype THEN 2147483647 + ELSE 9223372036854775807 + END AS type_max + FROM pg_attribute AS a + JOIN pg_class AS c ON c.oid = a.attrelid + JOIN pg_namespace AS n ON n.oid = c.relnamespace + WHERE a.attnum > 0 AND NOT a.attisdropped +), +fed AS ( + SELECT s.*, col.table_name, col.attribute, col.column_name, + least(s.seqmax, coalesce(col.type_max, s.seqmax)) AS capacity + FROM sequences AS s + LEFT JOIN feeds AS f ON f.seqrelid = s.seqrelid + -- The column is the limit only if it is no wider than its sequence. A bigint column fed by an + -- integer sequence (left behind by ALTER COLUMN TYPE bigint) needs the sequence changed, not the table. + LEFT JOIN columns AS col ON col.attrelid = f.table_oid AND col.attnum = f.attnum AND col.type_max <= s.seqmax +) +-- Sequences, measured against the column they feed, or their own maximum. +SELECT coalesce(fed.column_name, fed.sequence_name) AS subject, + CASE WHEN fed.last_value >= fed.capacity * @critical_ratio THEN 'critical' END AS severity, + fed.column_name, + CASE WHEN fed.column_name IS NULL THEN fed.sequence_name END AS sequence_name, + fed.table_name, + fed.attribute, + -- A serial column's sequence is often integer too, and needs changing with it. + CASE WHEN fed.column_name IS NOT NULL AND fed.seqmax <= 2147483647 THEN fed.sequence_name END AS owned_sequence, + NULL::text AS fk_column, + NULL::text AS referenced, + fed.last_value AS used, + fed.capacity +FROM fed +WHERE fed.last_value >= fed.capacity * @warning_ratio +UNION ALL +-- Narrower foreign keys that point at a column a sequence feeds. +SELECT fk.column_name, + CASE WHEN fed.last_value >= fk.type_max * @critical_ratio THEN 'critical' END, + NULL, + NULL, + fk.table_name, + fk.attribute, + NULL, + fk.column_name, + target.column_name, + fed.last_value, + fk.type_max +FROM pg_constraint AS k +JOIN columns AS fk ON fk.attrelid = k.conrelid AND fk.attnum = k.conkey[1] +JOIN columns AS target ON target.attrelid = k.confrelid AND target.attnum = k.confkey[1] +JOIN feeds AS f ON f.table_oid = k.confrelid AND f.attnum = k.confkey[1] +JOIN fed ON fed.seqrelid = f.seqrelid +WHERE k.contype = 'f' + AND cardinality(k.conkey) = 1 + AND fk.type_max < target.type_max + AND fed.last_value >= fk.type_max * @warning_ratio +ORDER BY 1 diff --git a/checks/integer-exhaustion/fixtures/fires.sql b/checks/integer-exhaustion/fixtures/fires.sql new file mode 100644 index 0000000..84abf55 --- /dev/null +++ b/checks/integer-exhaustion/fixtures/fires.sql @@ -0,0 +1,12 @@ +-- A serial column past 90% of integer, and an integer foreign key to a bigint key past 70%. +CREATE TABLE fixture_orders (id serial PRIMARY KEY); +SELECT setval('fixture_orders_id_seq', 2000000000); +CREATE TABLE fixture_invoices (id bigint GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY); +SELECT setval(pg_get_serial_sequence('fixture_invoices', 'id'), 1500000000); +CREATE TABLE fixture_payments (invoice_id int REFERENCES fixture_invoices (id)); +-- A column moved to bigint whose sequence stayed integer. +CREATE TABLE fixture_migrated (id serial); +ALTER TABLE fixture_migrated ALTER COLUMN id TYPE bigint; +SELECT setval('fixture_migrated_id_seq', 2000000000); +-- As pgcheckup grant prints it, so the check can read the counters. +GRANT SELECT ON ALL SEQUENCES IN SCHEMA public TO checkup; diff --git a/checks/integer-exhaustion/fixtures/healthy.sql b/checks/integer-exhaustion/fixtures/healthy.sql new file mode 100644 index 0000000..ec78817 --- /dev/null +++ b/checks/integer-exhaustion/fixtures/healthy.sql @@ -0,0 +1,11 @@ +-- The same high counters, on bigint columns all the way through. +CREATE TABLE fixture_orders (id bigserial PRIMARY KEY); +SELECT setval('fixture_orders_id_seq', 2000000000); +CREATE TABLE fixture_invoices (id bigint GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY); +SELECT setval(pg_get_serial_sequence('fixture_invoices', 'id'), 1500000000); +CREATE TABLE fixture_payments (invoice_id bigint REFERENCES fixture_invoices (id)); +-- A cycling sequence near its maximum starts over by design. +CREATE SEQUENCE fixture_tickets AS integer MAXVALUE 9999 CYCLE; +SELECT setval('fixture_tickets', 9990); +-- As pgcheckup grant prints it, so the check can read the counters. +GRANT SELECT ON ALL SEQUENCES IN SCHEMA public TO checkup; diff --git a/checks/invalid-index/check.md b/checks/invalid-index/check.md new file mode 100644 index 0000000..21e3410 --- /dev/null +++ b/checks/invalid-index/check.md @@ -0,0 +1,35 @@ +--- +id: invalid-index +title: Invalid index +category: capacity +severity: warning +min_version: 14 +privileges: [pg_read_all_stats] +message: "Index {subject} on {table_name} is invalid, usually left by a failed CREATE INDEX CONCURRENTLY. Queries never use it, but writes may still pay to maintain it." +fix: | + rebuild it, or drop it if it isn't needed: + REINDEX INDEX CONCURRENTLY {subject}; -- or DROP INDEX CONCURRENTLY {subject}; +--- + +## What breaks + +`CREATE INDEX CONCURRENTLY` builds an index without blocking writes, but if it fails (a duplicate value for a unique index, a deadlock, a cancelled migration), it leaves the index behind, marked invalid. The planner never uses it, so the queries it was built for keep scanning the table, which can overload the database once traffic grows. Meanwhile, writes may still keep it up to date, and an invalid unique index still rejects duplicates. + +Migrations that retry often leave one invalid index per failed attempt. + +The check ignores indexes still being built, and the parent index of a partitioned table, which stays invalid until every partition has its own. + +## Fix + +Rebuild it, or drop it if the migration that created it was abandoned: + +```sql +REINDEX INDEX CONCURRENTLY public.accounts_email; +DROP INDEX CONCURRENTLY public.accounts_email; +``` + +If it was a unique index, remove the duplicate rows first, or the rebuild fails the same way. + +## Seen in + +- [Building indexes concurrently](https://www.postgresql.org/docs/current/sql-createindex.html#SQL-CREATEINDEX-CONCURRENTLY), PostgreSQL documentation diff --git a/checks/invalid-index/check.sql b/checks/invalid-index/check.sql new file mode 100644 index 0000000..bb8d65c --- /dev/null +++ b/checks/invalid-index/check.sql @@ -0,0 +1,12 @@ +SELECT format('%I.%I', n.nspname, i.relname) AS subject, + format('%I.%I', n.nspname, t.relname) AS table_name +FROM pg_index AS x +JOIN pg_class AS i ON i.oid = x.indexrelid +JOIN pg_class AS t ON t.oid = x.indrelid +JOIN pg_namespace AS n ON n.oid = i.relnamespace +WHERE NOT x.indisvalid + -- A partitioned table's index stays invalid until every partition has its own. + AND i.relkind <> 'I' + -- An index still being built is invalid until the build finishes. + AND NOT EXISTS (SELECT FROM pg_stat_progress_create_index AS p WHERE p.index_relid = x.indexrelid) +ORDER BY 1 diff --git a/checks/invalid-index/fixtures/fires.sql b/checks/invalid-index/fixtures/fires.sql new file mode 100644 index 0000000..e9fc03d --- /dev/null +++ b/checks/invalid-index/fixtures/fires.sql @@ -0,0 +1,5 @@ +-- A unique index over duplicate values fails half way and stays behind, invalid. +CREATE TABLE fixture_accounts (email text); +INSERT INTO fixture_accounts VALUES ('ada@example.com'), ('ada@example.com'); +-- expect error +CREATE UNIQUE INDEX CONCURRENTLY fixture_accounts_email ON fixture_accounts (email); diff --git a/checks/invalid-index/fixtures/healthy.sql b/checks/invalid-index/fixtures/healthy.sql new file mode 100644 index 0000000..1a8cd82 --- /dev/null +++ b/checks/invalid-index/fixtures/healthy.sql @@ -0,0 +1,3 @@ +CREATE TABLE fixture_accounts (email text); +INSERT INTO fixture_accounts VALUES ('ada@example.com'), ('grace@example.com'); +CREATE UNIQUE INDEX CONCURRENTLY fixture_accounts_email ON fixture_accounts (email); diff --git a/checks/long-transaction/check.md b/checks/long-transaction/check.md new file mode 100644 index 0000000..67108b1 --- /dev/null +++ b/checks/long-transaction/check.md @@ -0,0 +1,39 @@ +--- +id: long-transaction +title: Long or idle transaction +category: cleanup +severity: warning +min_version: 14 +privileges: [pg_read_all_stats] +thresholds: + min_duration: 1h + min_idle: 10min +message: "Session {subject}[ ({role_name} on {database})] has had a transaction open for {open_for}[ and has been idle in it for {idle_for}]." +fix: | + if the session is stuck or abandoned, end it: + SELECT pg_terminate_backend({subject}); +--- + +## What breaks + +While a transaction is open, vacuum can't remove any row that the transaction might still see, anywhere in the database. Tables and indexes bloat, queries slow down, and transaction IDs can't be frozen, which leads toward wraparound. The transaction's locks also stay held, and a schema change that queues behind them blocks the traffic behind it. + +A session that is idle inside a transaction is the usual culprit: an application that began a transaction and never committed, or a console left open. It does nothing, but holds everything back. + +Backups with `pg_dump` also keep a transaction open for their whole run. That is expected, but it has the same effect on a busy database. + +## Fix + +Find out what the session is and whether it is still needed. If it is stuck or abandoned, end it: + +```sql +SELECT pg_terminate_backend(4127); +``` + +To stop sessions from idling in a transaction for good, set `idle_in_transaction_session_timeout` (see `idle-transaction-timeout`). + +## Seen in + +- [Post-mortem: service disruption on January 21–22, 2020](https://www.figma.com/blog/post-mortem-service-disruption-on-january-21-22-2020/), Figma +- [How we upgraded our 4 TB main application Postgres database](https://retool.com/blog/how-we-upgraded-postgresql-database), Retool +- [Zero-downtime Postgres migrations: the hard parts](https://gocardless.com/blog/zero-downtime-postgres-migrations-the-hard-parts/), GoCardless diff --git a/checks/long-transaction/check.sql b/checks/long-transaction/check.sql new file mode 100644 index 0000000..3d7f562 --- /dev/null +++ b/checks/long-transaction/check.sql @@ -0,0 +1,11 @@ +SELECT a.pid::text AS subject, + a.usename AS role_name, + a.datname AS database, + now() - a.xact_start AS open_for, + CASE WHEN a.state LIKE 'idle in transaction%' THEN now() - a.state_change END AS idle_for +FROM pg_stat_activity AS a +WHERE a.backend_type = 'client backend' + AND a.pid <> pg_backend_pid() + AND (a.xact_start < now() - @min_duration + OR (a.state LIKE 'idle in transaction%' AND a.state_change < now() - @min_idle)) +ORDER BY a.xact_start diff --git a/checks/long-transaction/fixtures/fires.sql b/checks/long-transaction/fixtures/fires.sql new file mode 100644 index 0000000..bd5ae3d --- /dev/null +++ b/checks/long-transaction/fixtures/fires.sql @@ -0,0 +1,5 @@ +-- threshold min_duration = 0s +-- threshold min_idle = 0s +-- The fixture's own session keeps this transaction open while the check runs. +BEGIN; +SELECT txid_current(); diff --git a/checks/long-transaction/fixtures/healthy.sql b/checks/long-transaction/fixtures/healthy.sql new file mode 100644 index 0000000..960b7e0 --- /dev/null +++ b/checks/long-transaction/fixtures/healthy.sql @@ -0,0 +1,6 @@ +-- threshold min_duration = 0s +-- threshold min_idle = 0s +-- A transaction that has already committed holds nothing back. +BEGIN; +SELECT txid_current(); +COMMIT; diff --git a/checks/multixact-wraparound/check.md b/checks/multixact-wraparound/check.md new file mode 100644 index 0000000..35c623c --- /dev/null +++ b/checks/multixact-wraparound/check.md @@ -0,0 +1,36 @@ +--- +id: multixact-wraparound +title: Multixact ID wraparound +category: ids +severity: warning +min_version: 14 +privileges: [] +thresholds: + warning_age: 500000000 + critical_age: 1500000000 +message: "[Database {database}][Table {table_name}][Temporary table {temp_table}] has used {mxid_age:count} of its 2.1 billion multixact IDs." +fix: "[connect to {database} and scan it to find its oldest tables, then freeze them.][VACUUM (FREEZE, VERBOSE) {table_name};][only the session that created {temp_table} can vacuum it, so have it drop the table, or end that session.]" +--- + +## What breaks + +When more than one transaction locks the same row, as foreign keys and `SELECT … FOR SHARE` do, Postgres records the group as a multixact, with its own 32-bit ID. Like transaction IDs, multixact IDs wrap around. Vacuum has to freeze old ones, and if a table goes about 2.1 billion multixacts without that, Postgres stops accepting writes. + +Autovacuum starts an anti-wraparound vacuum at 400 million by default (`autovacuum_multixact_freeze_max_age`). Workloads with many foreign keys, or many concurrent row locks, use multixacts fastest. + +This check measures tables in the database it connects to, and every other database in the cluster as a whole. It lists at most the 20 oldest tables, including temporary ones: only the session that created a temporary table can vacuum it, so a pooled connection that keeps one for months ages the whole database. + +## Fix + +Freeze the oldest tables first: + +```sql +VACUUM (FREEZE, VERBOSE) public.accounts; +``` + +As with transaction IDs, a long transaction, a forgotten prepared transaction or an inactive replication slot can hold vacuum back. pgcheckup's other checks look for them. + +## Seen in + +- [Root cause analysis: PostgreSQL MultiXact member exhaustion incidents](https://metronome.com/blog/root-cause-analysis-postgresql-multixact-member-exhaustion-incidents-may-2025), Metronome +- [Multixacts and wraparound](https://www.postgresql.org/docs/current/routine-vacuuming.html#VACUUM-FOR-MULTIXACT-WRAPAROUND), PostgreSQL documentation diff --git a/checks/multixact-wraparound/check.sql b/checks/multixact-wraparound/check.sql new file mode 100644 index 0000000..c676c3e --- /dev/null +++ b/checks/multixact-wraparound/check.sql @@ -0,0 +1,29 @@ +-- Other databases in the cluster, as a whole. Those that don't allow connections (template0) +-- are frozen by autovacuum itself. +SELECT d.datname AS subject, + CASE WHEN mxid_age(d.datminmxid) >= @critical_age THEN 'critical' END AS severity, + d.datname AS database, + NULL::text AS table_name, + NULL::text AS temp_table, + mxid_age(d.datminmxid)::bigint AS mxid_age +FROM pg_database AS d +WHERE d.datallowconn + AND d.datname <> current_database() + AND mxid_age(d.datminmxid) >= @warning_age +UNION ALL +-- Tables in this database. A TOAST table is vacuumed with its table, so it counts toward it. +SELECT * FROM ( + SELECT format('%I.%I', n.nspname, c.relname), + CASE WHEN greatest(mxid_age(c.relminmxid), mxid_age(t.relminmxid)) >= @critical_age THEN 'critical' END, + NULL::text, + CASE WHEN c.relpersistence <> 't' THEN format('%I.%I', n.nspname, c.relname) END, + CASE WHEN c.relpersistence = 't' THEN format('%I.%I', n.nspname, c.relname) END, + greatest(mxid_age(c.relminmxid), mxid_age(t.relminmxid))::bigint + FROM pg_class AS c + JOIN pg_namespace AS n ON n.oid = c.relnamespace + LEFT JOIN pg_class AS t ON t.oid = c.reltoastrelid + WHERE c.relkind IN ('r', 'm') + AND greatest(mxid_age(c.relminmxid), mxid_age(t.relminmxid)) >= @warning_age + ORDER BY 6 DESC + LIMIT 20 +) AS oldest diff --git a/checks/multixact-wraparound/fixtures/fires.sql b/checks/multixact-wraparound/fixtures/fires.sql new file mode 100644 index 0000000..6f95926 --- /dev/null +++ b/checks/multixact-wraparound/fixtures/fires.sql @@ -0,0 +1,12 @@ +-- threshold warning_age = 1 +-- Locking a row, then updating it in a savepoint, makes one multixact, which ages every table +-- that hasn't been frozen since. +CREATE TABLE fixture_accounts (id int PRIMARY KEY, balance int); +-- The fixture's session keeps this until the check has run. +CREATE TEMP TABLE fixture_scratch (id int); +INSERT INTO fixture_accounts VALUES (1, 0); +BEGIN; +SELECT * FROM fixture_accounts FOR SHARE; +SAVEPOINT fixture; +UPDATE fixture_accounts SET balance = 1; +COMMIT; diff --git a/checks/multixact-wraparound/fixtures/healthy.sql b/checks/multixact-wraparound/fixtures/healthy.sql new file mode 100644 index 0000000..fd4b522 --- /dev/null +++ b/checks/multixact-wraparound/fixtures/healthy.sql @@ -0,0 +1,5 @@ +-- threshold warning_age = 1 +-- With no multixact made, nothing has aged, even at the lowest threshold. +CREATE TABLE fixture_accounts (id int PRIMARY KEY, balance int); +INSERT INTO fixture_accounts VALUES (1, 0); +UPDATE fixture_accounts SET balance = 1; diff --git a/checks/postgres-eol/check.md b/checks/postgres-eol/check.md new file mode 100644 index 0000000..c8cd0e9 --- /dev/null +++ b/checks/postgres-eol/check.md @@ -0,0 +1,27 @@ +--- +id: postgres-eol +title: Postgres version near or past end of life +category: capacity +severity: warning +min_version: 10 +privileges: [] +thresholds: + warn_before: 90d +message: "PostgreSQL {subject}[ reached end of life on {ended_on} and gets no more bug or security fixes][ reaches end of life on {ends_on}, in {time_left}]." +fix: plan an upgrade to PostgreSQL {latest} with pg_upgrade or logical replication, and rehearse it on a copy first. +--- + +## What breaks + +Each major version of Postgres gets fixes for five years. After its end-of-life date, security holes and data-corruption bugs found in it stay open, and extensions and managed providers drop it. Managed providers also force an upgrade on their own schedule, or charge for extended support. + +The check warns once a version is past its end of life, and gives notice (`info`) from 90 days before (`warn_before`). The dates ship inside each pgcheckup release, and the check compares them with the server's clock. + +## Fix + +Upgrade to a supported major version. `pg_upgrade` is fastest on a self-managed server; logical replication keeps downtime shortest. Either way, rehearse on a copy first: extensions and query plans can change between versions. + +## Seen in + +- [How we upgraded our 4 TB main application Postgres database](https://retool.com/blog/how-we-upgraded-postgresql-database), Retool +- [Versioning policy](https://www.postgresql.org/support/versioning/), PostgreSQL diff --git a/checks/postgres-eol/check.sql b/checks/postgres-eol/check.sql new file mode 100644 index 0000000..619d29c --- /dev/null +++ b/checks/postgres-eol/check.sql @@ -0,0 +1,15 @@ +-- End-of-life dates from https://www.postgresql.org/support/versioning/. Add each new major here. +WITH eol(major, ends) AS ( + VALUES (10, date '2022-11-10'), (11, date '2023-11-09'), (12, date '2024-11-21'), + (13, date '2025-11-13'), (14, date '2026-11-12'), (15, date '2027-11-11'), + (16, date '2028-11-09'), (17, date '2029-11-08'), (18, date '2030-11-14') +) +SELECT e.major::text AS subject, + CASE WHEN e.ends > now() THEN 'info' END AS severity, + CASE WHEN e.ends <= now() THEN to_char(e.ends, 'YYYY-MM-DD') END AS ended_on, + CASE WHEN e.ends > now() THEN to_char(e.ends, 'YYYY-MM-DD') END AS ends_on, + CASE WHEN e.ends > now() THEN (e.ends - current_date) * interval '1 day' END AS time_left, + (SELECT max(major) FROM eol) AS latest +FROM eol AS e +WHERE e.major = current_setting('server_version_num')::int / 10000 + AND e.ends - @warn_before <= now() diff --git a/checks/postgres-eol/fixtures/fires.sql b/checks/postgres-eol/fixtures/fires.sql new file mode 100644 index 0000000..895911e --- /dev/null +++ b/checks/postgres-eol/fixtures/fires.sql @@ -0,0 +1,3 @@ +-- threshold warn_before = 36500d +-- A century of notice puts every supported version within reach of its end of life. +SELECT 1; diff --git a/checks/postgres-eol/fixtures/healthy.sql b/checks/postgres-eol/fixtures/healthy.sql new file mode 100644 index 0000000..f28900c --- /dev/null +++ b/checks/postgres-eol/fixtures/healthy.sql @@ -0,0 +1,4 @@ +-- threshold warn_before = 0s +-- A supported version, with no notice, has nothing to report. Once Postgres 14 passes its end +-- of life (2026-11-12), this fails on 14, which is the signal to drop it from the CI matrix. +SELECT 1; diff --git a/checks/prepared-transaction-orphaned/check.md b/checks/prepared-transaction-orphaned/check.md new file mode 100644 index 0000000..542fae5 --- /dev/null +++ b/checks/prepared-transaction-orphaned/check.md @@ -0,0 +1,35 @@ +--- +id: prepared-transaction-orphaned +title: Orphaned prepared transaction +category: cleanup +severity: warning +min_version: 14 +privileges: [] +thresholds: + min_age: 5min +message: "Prepared transaction {subject} in {database} has waited {waiting_for} for a COMMIT PREPARED or ROLLBACK PREPARED, holding its locks and holding back vacuum." +fix: | + if the coordinator that prepared it is gone, finish it by hand in {database}: + COMMIT PREPARED {gid_literal}; -- or ROLLBACK PREPARED {gid_literal}; +--- + +## What breaks + +A prepared transaction is the first half of a two-phase commit. It survives disconnects and restarts until someone commits or rolls it back. If the coordinator that prepared it crashes or forgets it, the transaction stays forever: its row locks keep blocking writes, and vacuum can't clean up after it, which leads toward transaction ID wraparound. + +It doesn't show up in `pg_stat_activity`, because no session holds it, so it is easy to miss. + +Two-phase commit is off unless `max_prepared_transactions` is above zero, so on most servers this check finds nothing. + +## Fix + +Find out from the coordinator (a transaction manager, a message queue, an application server) whether the transaction should commit or roll back, then finish it in its database: + +```sql +COMMIT PREPARED 'fixture'; +ROLLBACK PREPARED 'fixture'; +``` + +## Seen in + +- [PREPARE TRANSACTION](https://www.postgresql.org/docs/current/sql-prepare-transaction.html), PostgreSQL documentation diff --git a/checks/prepared-transaction-orphaned/check.sql b/checks/prepared-transaction-orphaned/check.sql new file mode 100644 index 0000000..bb2b1bd --- /dev/null +++ b/checks/prepared-transaction-orphaned/check.sql @@ -0,0 +1,7 @@ +SELECT p.gid AS subject, + p.database, + quote_literal(p.gid) AS gid_literal, + now() - p.prepared AS waiting_for +FROM pg_prepared_xacts AS p +WHERE p.prepared < now() - @min_age +ORDER BY p.prepared diff --git a/checks/prepared-transaction-orphaned/fixtures/fires.sql b/checks/prepared-transaction-orphaned/fixtures/fires.sql new file mode 100644 index 0000000..cde2926 --- /dev/null +++ b/checks/prepared-transaction-orphaned/fixtures/fires.sql @@ -0,0 +1,6 @@ +-- server max_prepared_transactions = 5 +-- threshold min_age = 0s +CREATE TABLE fixture_payments (id int); +BEGIN; +INSERT INTO fixture_payments VALUES (1); +PREPARE TRANSACTION 'fixture'; diff --git a/checks/prepared-transaction-orphaned/fixtures/healthy.sql b/checks/prepared-transaction-orphaned/fixtures/healthy.sql new file mode 100644 index 0000000..f0623c5 --- /dev/null +++ b/checks/prepared-transaction-orphaned/fixtures/healthy.sql @@ -0,0 +1,8 @@ +-- server max_prepared_transactions = 5 +-- threshold min_age = 0s +-- A prepared transaction that was committed is gone. +CREATE TABLE fixture_payments (id int); +BEGIN; +INSERT INTO fixture_payments VALUES (1); +PREPARE TRANSACTION 'fixture'; +COMMIT PREPARED 'fixture'; diff --git a/checks/replication-slot-inactive/check.md b/checks/replication-slot-inactive/check.md index 2c013b6..d44dfa2 100644 --- a/checks/replication-slot-inactive/check.md +++ b/checks/replication-slot-inactive/check.md @@ -7,7 +7,8 @@ min_version: 14 privileges: [] thresholds: min_retained_wal: 1GB -message: Slot {subject} has been inactive[ for {inactive_for}] and is holding {retained_wal:bytes} of WAL. + min_xmin_age: 100000000 +message: "Slot {subject} has been inactive[ for {inactive_for}][ and is holding {retained_wal:bytes} of WAL].[ Vacuum can't clean up after the last {xmin_age:count} transactions while it exists.]" fix: | restart its consumer, or drop the slot: SELECT pg_drop_replication_slot({slot_literal}); @@ -17,7 +18,7 @@ fix: | A replication slot makes Postgres keep every WAL segment that its consumer hasn't confirmed. When the consumer stops, the slot keeps WAL for as long as it exists. Typical consumers are a replica that was removed, a paused CDC connector such as Debezium, or a subscription dropped without its slot. `pg_wal` grows until the disk is full, and then Postgres stops accepting writes. -A logical slot also holds back `catalog_xmin`, so vacuum can't clean up the system catalogs while the slot waits. +A slot can also hold back vacuum while keeping little WAL. A logical slot pins `catalog_xmin`, and a physical slot used with `hot_standby_feedback` pins `xmin`. Vacuum can't clean up after any transaction since, which bloats tables and leads toward transaction ID wraparound. The check reports that from 100 million transactions (`min_xmin_age`). ## Fix @@ -31,5 +32,6 @@ Then cap the WAL any slot can keep with `max_slot_wal_keep_size`. A slot that pa ## Seen in +- [Postgres almost-outage postmortem: the hidden dangers of replication slots and autovacuum](https://dev.to/sasikumart/postgres-almost-outage-postmortem-the-hidden-dangers-of-replication-slots-and-autovacuum-2nem), DEV Community - [The Insatiable Postgres Replication Slot](https://www.morling.dev/blog/insatiable-postgres-replication-slot/), Gunnar Morling: an inactive slot on an idle Amazon RDS database kept growing its WAL. - [Replication slots](https://www.postgresql.org/docs/current/warm-standby.html#STREAMING-REPLICATION-SLOTS), PostgreSQL documentation: "replication slots can cause the server to retain so many WAL segments that they fill up the space allocated for pg_wal." diff --git a/checks/replication-slot-inactive/check.sql b/checks/replication-slot-inactive/check.sql index 070362f..ce4223c 100644 --- a/checks/replication-slot-inactive/check.sql +++ b/checks/replication-slot-inactive/check.sql @@ -3,13 +3,15 @@ SELECT s.slot_name AS subject, -- inactive_since arrived in Postgres 17. Reading it through to_jsonb keeps one query -- for every supported version, and gives NULL before 17. now() - (to_jsonb(s) ->> 'inactive_since')::timestamptz AS inactive_for, - w.retained_wal + w.retained_wal, + CASE WHEN w.xmin_age >= @min_xmin_age THEN w.xmin_age END AS xmin_age FROM pg_replication_slots AS s CROSS JOIN LATERAL ( -- On a standby, pg_current_wal_lsn() raises an error; the replay position is its equivalent. SELECT pg_wal_lsn_diff( CASE WHEN pg_is_in_recovery() THEN pg_last_wal_replay_lsn() ELSE pg_current_wal_lsn() END, - s.restart_lsn)::bigint AS retained_wal + s.restart_lsn)::bigint AS retained_wal, + greatest(age(s.xmin), age(s.catalog_xmin))::bigint AS xmin_age ) AS w WHERE NOT s.active -- A lost slot has already been invalidated and holds no WAL. @@ -17,5 +19,5 @@ WHERE NOT s.active -- On a standby, a slot synced from the primary (Postgres 17 and later) always looks inactive, -- and can't be dropped there. Its consumer is on the primary, where this check covers it. AND NOT coalesce((to_jsonb(s) ->> 'synced')::boolean, false) - AND w.retained_wal >= @min_retained_wal -ORDER BY w.retained_wal DESC + AND (w.retained_wal >= @min_retained_wal OR w.xmin_age >= @min_xmin_age) +ORDER BY w.retained_wal DESC NULLS LAST diff --git a/checks/replication-slot-inactive/fixtures/fires.sql b/checks/replication-slot-inactive/fixtures/fires.sql index 0bb7edc..723ee32 100644 --- a/checks/replication-slot-inactive/fixtures/fires.sql +++ b/checks/replication-slot-inactive/fixtures/fires.sql @@ -1,4 +1,9 @@ --- threshold min_retained_wal = 0B --- A physical slot that reserves WAL from the start and never gets a consumer. -SELECT pg_create_physical_replication_slot('fixture_slot', true); -CREATE TABLE fixture_wal AS SELECT g FROM generate_series(1, 1000) AS g; +-- server wal_level = logical +-- threshold min_retained_wal = 1TB +-- threshold min_xmin_age = 1 +-- A logical slot pins catalog_xmin from the moment it is created, and ages as transactions +-- go by. It keeps little WAL, so only the xmin half of the check fires. The WAL half is covered +-- by the scan tests, which fill a slot with more than 1 GB. +SELECT pg_create_logical_replication_slot('fixture_slot', 'pgoutput'); +SELECT txid_current(); +SELECT txid_current(); diff --git a/checks/replication-slot-inactive/fixtures/healthy.sql b/checks/replication-slot-inactive/fixtures/healthy.sql index 74b0433..6de479e 100644 --- a/checks/replication-slot-inactive/fixtures/healthy.sql +++ b/checks/replication-slot-inactive/fixtures/healthy.sql @@ -1,4 +1,6 @@ -- threshold min_retained_wal = 0B --- A slot that has never reserved WAL holds none back, so even a zero threshold stays quiet. +-- threshold min_xmin_age = 1 +-- A slot that has never reserved WAL holds none back, and pins no xmin, so even the lowest +-- thresholds stay quiet. SELECT pg_create_physical_replication_slot('fixture_slot'); CREATE TABLE fixture_wal AS SELECT g FROM generate_series(1, 1000) AS g; diff --git a/checks/replication-slot-unbounded/check.md b/checks/replication-slot-unbounded/check.md new file mode 100644 index 0000000..8144578 --- /dev/null +++ b/checks/replication-slot-unbounded/check.md @@ -0,0 +1,37 @@ +--- +id: replication-slot-unbounded +title: Replication slots without a WAL limit +category: wal +severity: warning +min_version: 14 +privileges: [] +message: "{subject} is -1 and this server has replication slots, so one stuck slot can keep WAL until the disk fills." +fix: | + cap it below the free space on the WAL disk: + ALTER SYSTEM SET max_slot_wal_keep_size = '50GB'; + SELECT pg_reload_conf(); +--- + +## What breaks + +A replication slot keeps every WAL segment its consumer hasn't confirmed. With `max_slot_wal_keep_size = -1`, the default, there is no limit: one consumer that stops, a CDC connector or a replica, makes `pg_wal` grow until the disk is full and Postgres stops accepting writes. `replication-slot-inactive` reports such a slot once it has stopped; this limit keeps it from taking the server down. + +With a limit, a slot that falls too far behind is invalidated instead. Its consumer then has to resynchronize, which is much cheaper than an outage. + +The check only fires when the server has at least one slot, because without slots the setting doesn't matter. On Postgres 18, `idle_replication_slot_timeout` also counts as a limit. + +## Fix + +Choose a size below the free space on the disk that holds `pg_wal`, with room to spare: + +```sql +ALTER SYSTEM SET max_slot_wal_keep_size = '50GB'; +SELECT pg_reload_conf(); +``` + +On a managed provider, set it in the parameter group or its equivalent. + +## Seen in + +- [Postgres almost-outage postmortem: the hidden dangers of replication slots and autovacuum](https://dev.to/sasikumart/postgres-almost-outage-postmortem-the-hidden-dangers-of-replication-slots-and-autovacuum-2nem), DEV Community +- [max_slot_wal_keep_size](https://www.postgresql.org/docs/current/runtime-config-replication.html#GUC-MAX-SLOT-WAL-KEEP-SIZE), PostgreSQL documentation diff --git a/checks/replication-slot-unbounded/check.sql b/checks/replication-slot-unbounded/check.sql new file mode 100644 index 0000000..b764892 --- /dev/null +++ b/checks/replication-slot-unbounded/check.sql @@ -0,0 +1,5 @@ +SELECT 'max_slot_wal_keep_size' AS subject +WHERE current_setting('max_slot_wal_keep_size') = '-1' + -- idle_replication_slot_timeout arrived in Postgres 18, and caps idle slots too. + AND coalesce(current_setting('idle_replication_slot_timeout', true), '0') = '0' + AND EXISTS (SELECT FROM pg_replication_slots) diff --git a/checks/replication-slot-unbounded/fixtures/fires.sql b/checks/replication-slot-unbounded/fixtures/fires.sql new file mode 100644 index 0000000..840350f --- /dev/null +++ b/checks/replication-slot-unbounded/fixtures/fires.sql @@ -0,0 +1,2 @@ +-- Stock Postgres has no limit, and this slot makes it matter. +SELECT pg_create_physical_replication_slot('fixture_slot'); diff --git a/checks/replication-slot-unbounded/fixtures/healthy.sql b/checks/replication-slot-unbounded/fixtures/healthy.sql new file mode 100644 index 0000000..e53cf98 --- /dev/null +++ b/checks/replication-slot-unbounded/fixtures/healthy.sql @@ -0,0 +1,2 @@ +-- server max_slot_wal_keep_size = 10GB +SELECT pg_create_physical_replication_slot('fixture_slot'); diff --git a/checks/wal-archiving-failing/check.md b/checks/wal-archiving-failing/check.md new file mode 100644 index 0000000..9894635 --- /dev/null +++ b/checks/wal-archiving-failing/check.md @@ -0,0 +1,34 @@ +--- +id: wal-archiving-failing +title: WAL archiving failing +category: wal +severity: warning +min_version: 14 +privileges: [pg_monitor] +skip_on: [rds, aurora, cloudsql, azure, supabase, neon] +thresholds: + min_duration: 1h +message: "[archive_mode is {unset_mode}, but no archive_command is set, so WAL piles up in pg_wal and none of it is archived.][WAL archiving is failing, and the oldest segment has waited {failing_for}, so point-in-time recovery has a gap and WAL piles up in pg_wal.][The oldest of {ready_count:count} WAL segments has waited {waiting_for} to be archived, so the archiver seems stuck.]" +fix: "read the server log for the archiver's errors. Set archive_command if it is empty, fix it or its destination if it fails or hangs (a full or unreachable destination is typical), or turn archive_mode off if you don't archive." +--- + +## What breaks + +With `archive_mode` on, Postgres keeps each WAL segment until `archive_command` has copied it somewhere safe. If archiving fails or stops, two things go wrong: + +- Point-in-time recovery has a gap from the last segment archived. A restore can't get past it. +- The segments that wait pile up in `pg_wal` until the disk is full, and Postgres stops accepting writes. + +The check reports three cases: `archive_mode` on without a command, a command that keeps failing, and an archiver that makes no progress without logging a failure. For the last two, a segment must have waited an hour (`min_duration`), so a brief failure that the archiver retries past isn't reported. It never prints `archive_command`, which can hold credentials. + +Managed providers archive WAL themselves, so the check is skipped there. + +## Fix + +Read the server log for the command's error, and fix the command or its destination. When the command succeeds again, Postgres archives the backlog on its own. If you don't need archiving, turn `archive_mode` off, which needs a restart. + +## Seen in + +- [PostgreSQL archiver failure](https://vsevolod.net/postgresql-archiver-failure/), Vsevolod +- [Postgres is out of disk and how to recover: the dos and don'ts](https://www.crunchydata.com/blog/postgres-is-out-of-disk-and-how-to-recover-the-dos-and-donts), Crunchy Data +- [Setting up WAL archiving](https://www.postgresql.org/docs/current/continuous-archiving.html#BACKUP-ARCHIVING-WAL), PostgreSQL documentation diff --git a/checks/wal-archiving-failing/check.sql b/checks/wal-archiving-failing/check.sql new file mode 100644 index 0000000..4a18025 --- /dev/null +++ b/checks/wal-archiving-failing/check.sql @@ -0,0 +1,36 @@ +WITH settings AS ( + SELECT current_setting('archive_mode') AS archive_mode, + -- archive_library arrived in Postgres 15. Neither value is ever returned: either can + -- hold credentials. + current_setting('archive_command') = '' + AND coalesce(current_setting('archive_library', true), '') = '' AS unset +), +archiver AS ( + SELECT coalesce(a.last_failed_time > coalesce(a.last_archived_time, '-infinity'), false) AS failing + FROM pg_stat_archiver AS a +), +ready AS ( + SELECT count(*) AS segments, min(modification) AS oldest + FROM pg_ls_archive_statusdir() + WHERE name LIKE '%.ready' +) +SELECT 'archive_command' AS subject, + s.archive_mode AS unset_mode, + NULL::interval AS failing_for, + NULL::bigint AS ready_count, + NULL::interval AS waiting_for +FROM settings AS s +WHERE s.archive_mode <> 'off' AND s.unset +UNION ALL +-- Both of these go by how long the oldest segment has waited: the archiver retries a brief +-- failure, and on a quiet server the last success can be long ago without anything wrong. +SELECT 'archiver', NULL, now() - r.oldest, NULL, NULL +FROM settings AS s, archiver AS a, ready AS r +WHERE s.archive_mode <> 'off' AND NOT s.unset AND a.failing + AND r.oldest < now() - @min_duration +UNION ALL +-- A hung archiver logs no failure, so only the age of the waiting segments shows it. +SELECT 'archive_status', NULL, NULL, r.segments, now() - r.oldest +FROM settings AS s, archiver AS a, ready AS r +WHERE s.archive_mode <> 'off' AND NOT s.unset AND NOT a.failing + AND r.oldest < now() - @min_duration diff --git a/checks/wal-archiving-failing/fixtures/fires.sql b/checks/wal-archiving-failing/fixtures/fires.sql new file mode 100644 index 0000000..3b97a40 --- /dev/null +++ b/checks/wal-archiving-failing/fixtures/fires.sql @@ -0,0 +1,16 @@ +-- server archive_mode = on +-- server archive_command = false +-- threshold min_duration = 0s +-- `false` always fails. Switching WAL gives the archiver a segment to try, then this waits +-- until the archiver has reported its first failure. +CREATE TABLE fixture_wal AS SELECT g FROM generate_series(1, 1000) AS g; +SELECT pg_switch_wal(); +DO $$ +BEGIN + FOR i IN 1..100 LOOP + PERFORM pg_stat_clear_snapshot(); + EXIT WHEN (SELECT failed_count FROM pg_stat_archiver) > 0; + PERFORM pg_sleep(0.1); + END LOOP; +END +$$; diff --git a/checks/wal-archiving-failing/fixtures/healthy.sql b/checks/wal-archiving-failing/fixtures/healthy.sql new file mode 100644 index 0000000..393c525 --- /dev/null +++ b/checks/wal-archiving-failing/fixtures/healthy.sql @@ -0,0 +1,14 @@ +-- server archive_mode = on +-- server archive_command = true +-- `true` always succeeds. This waits until the archiver has archived the switched segment. +CREATE TABLE fixture_wal AS SELECT g FROM generate_series(1, 1000) AS g; +SELECT pg_switch_wal(); +DO $$ +BEGIN + FOR i IN 1..100 LOOP + PERFORM pg_stat_clear_snapshot(); + EXIT WHEN (SELECT archived_count FROM pg_stat_archiver) > 0; + PERFORM pg_sleep(0.1); + END LOOP; +END +$$; diff --git a/checks/xid-wraparound/check.md b/checks/xid-wraparound/check.md new file mode 100644 index 0000000..8970218 --- /dev/null +++ b/checks/xid-wraparound/check.md @@ -0,0 +1,40 @@ +--- +id: xid-wraparound +title: Transaction ID wraparound +category: ids +severity: warning +min_version: 14 +privileges: [] +thresholds: + warning_age: 500000000 + critical_age: 1500000000 +message: "[Database {database}][Table {table_name}][Temporary table {temp_table}] has used {xid_age:count} of its 2.1 billion transaction IDs." +fix: "[connect to {database} and scan it to find its oldest tables, then freeze them.][VACUUM (FREEZE, VERBOSE) {table_name};][only the session that created {temp_table} can vacuum it, so have it drop the table, or end that session.]" +--- + +## What breaks + +Postgres numbers transactions with 32-bit IDs and reuses them in a circle. Vacuum marks old rows as frozen so that reuse is safe. If a table goes about 2.1 billion transactions without being frozen, Postgres stops accepting writes to protect the data, and the fix is a long single-user vacuum while the application is down. + +Long before that, at 200 million by default (`autovacuum_freeze_max_age`), autovacuum starts an anti-wraparound vacuum that can't be cancelled, and schema changes queue behind its lock. From 1.6 billion, Postgres switches to its failsafe mode. + +The usual cause is something that holds vacuum back: a long transaction, a forgotten prepared transaction or an inactive replication slot. The checks `long-transaction`, `prepared-transaction-orphaned` and `replication-slot-inactive` look for those. + +This check measures tables in the database it connects to, and every other database in the cluster as a whole. It lists at most the 20 oldest tables, including temporary ones: only the session that created a temporary table can vacuum it, so a pooled connection that keeps one for months ages the whole database. + +## Fix + +Freeze the oldest tables first: + +```sql +VACUUM (FREEZE, VERBOSE) public.orders; +``` + +If the age keeps climbing, find and end what holds vacuum back before vacuuming again. On a large table, the vacuum takes a while; let it finish rather than cancelling it. + +## Seen in + +- [Transaction ID wraparound in Postgres](https://blog.sentry.io/transaction-id-wraparound-in-postgres/), Sentry +- [What we learned from the recent Mandrill outage](https://mailchimp.com/what-we-learned-from-the-recent-mandrill-outage/), Mailchimp +- [Understanding an outage: concurrency control and vacuuming in PostgreSQL](https://duffel.com/blog/understanding-outage-concurrency-vacuum-postgresql), Duffel +- [Preventing transaction ID wraparound failures](https://www.postgresql.org/docs/current/routine-vacuuming.html#VACUUM-FOR-WRAPAROUND), PostgreSQL documentation diff --git a/checks/xid-wraparound/check.sql b/checks/xid-wraparound/check.sql new file mode 100644 index 0000000..4a29240 --- /dev/null +++ b/checks/xid-wraparound/check.sql @@ -0,0 +1,29 @@ +-- Other databases in the cluster, as a whole. Those that don't allow connections (template0) +-- are frozen by autovacuum itself. +SELECT d.datname AS subject, + CASE WHEN age(d.datfrozenxid) >= @critical_age THEN 'critical' END AS severity, + d.datname AS database, + NULL::text AS table_name, + NULL::text AS temp_table, + age(d.datfrozenxid)::bigint AS xid_age +FROM pg_database AS d +WHERE d.datallowconn + AND d.datname <> current_database() + AND age(d.datfrozenxid) >= @warning_age +UNION ALL +-- Tables in this database. A TOAST table is vacuumed with its table, so it counts toward it. +SELECT * FROM ( + SELECT format('%I.%I', n.nspname, c.relname), + CASE WHEN greatest(age(c.relfrozenxid), age(t.relfrozenxid)) >= @critical_age THEN 'critical' END, + NULL::text, + CASE WHEN c.relpersistence <> 't' THEN format('%I.%I', n.nspname, c.relname) END, + CASE WHEN c.relpersistence = 't' THEN format('%I.%I', n.nspname, c.relname) END, + greatest(age(c.relfrozenxid), age(t.relfrozenxid))::bigint + FROM pg_class AS c + JOIN pg_namespace AS n ON n.oid = c.relnamespace + LEFT JOIN pg_class AS t ON t.oid = c.reltoastrelid + WHERE c.relkind IN ('r', 'm') + AND greatest(age(c.relfrozenxid), age(t.relfrozenxid)) >= @warning_age + ORDER BY 6 DESC + LIMIT 20 +) AS oldest diff --git a/checks/xid-wraparound/fixtures/fires.sql b/checks/xid-wraparound/fixtures/fires.sql new file mode 100644 index 0000000..9aa5f47 --- /dev/null +++ b/checks/xid-wraparound/fixtures/fires.sql @@ -0,0 +1,9 @@ +-- threshold warning_age = 3 +-- Consuming transaction IDs ages every table that hasn't been frozen since. +CREATE TABLE fixture_orders (id int); +-- The fixture's session keeps this until the check has run. +CREATE TEMP TABLE fixture_scratch (id int); +SELECT txid_current(); +SELECT txid_current(); +SELECT txid_current(); +SELECT txid_current(); diff --git a/checks/xid-wraparound/fixtures/healthy.sql b/checks/xid-wraparound/fixtures/healthy.sql new file mode 100644 index 0000000..9c4dd49 --- /dev/null +++ b/checks/xid-wraparound/fixtures/healthy.sql @@ -0,0 +1,4 @@ +-- threshold warning_age = 10000 +-- Freshly frozen, every table here is young. The other databases of a new cluster are too. +CREATE TABLE fixture_orders (id int); +VACUUM (FREEZE); diff --git a/docs/json.md b/docs/json.md new file mode 100644 index 0000000..c5a6e02 --- /dev/null +++ b/docs/json.md @@ -0,0 +1,60 @@ +# JSON report + +`pgcheckup scan --format json` writes one JSON object. Its shape is versioned by `schema`. Adding a field doesn't change the version. Removing or renaming one, or changing what a value means, raises it. The exit codes are the same as for the terminal report. + +```json +{ + "schema": 1, + "pgcheckup": "0.1.0", + "server": { + "database": "app", + "host": "db.example.com", + "version": "17.6", + "provider": { "id": "rds", "name": "Amazon RDS" } + }, + "summary": { "passed": 11, "critical": 0, "warning": 1, "info": 0, "errored": 0, "skipped": 1 }, + "checks": [ + { + "id": "replication-slot-inactive", + "title": "Inactive replication slot", + "category": "wal", + "status": "warning", + "findings": [ + { + "subject": "debezium", + "severity": "warning", + "message": "Slot debezium has been inactive for 3 days and is holding 48 GB of WAL.", + "fix": "restart its consumer, or drop the slot:\nSELECT pg_drop_replication_slot('debezium');", + "values": { "subject": "debezium", "inactive_for": 259200, "retained_wal": 51539607552, "xmin_age": null } + } + ] + }, + { + "id": "wal-archiving-failing", + "title": "WAL archiving failing", + "category": "wal", + "status": "skipped", + "reason": "managed by Amazon RDS", + "findings": [] + } + ] +} +``` + +## Fields + +| Field | Meaning | +| --- | --- | +| `schema` | The version of this shape. Currently `1`. | +| `pgcheckup` | The version of pgcheckup that wrote the report. | +| `server.database`, `server.host` | The database scanned and the host as given. The connection string and password are never included. | +| `server.version` | The Postgres version, such as `17.6`. | +| `server.provider` | The managed service detected (`id` and `name`), or `null` for a self-managed server. | +| `summary` | How many checks passed, and how many ended at each severity. Each check counts once, at its worst finding. | +| `checks[].id` | The check's stable id. `pgcheckup explain ` describes it. | +| `checks[].status` | `passed`, `critical`, `warning`, `info` (its worst finding), `skipped` or `errored`. | +| `checks[].reason` | Why the check was skipped or errored. Only present for those two statuses. | +| `checks[].findings[].subject` | The object the finding is about, such as a slot or table name. | +| `checks[].findings[].severity` | `critical`, `warning` or `info`. | +| `checks[].findings[].message`, `.fix` | The text the terminal report prints. pgcheckup never runs the fix. | +| `checks[].findings[].values` | The facts behind the message, one per column it uses: sizes and counts as numbers, durations in seconds, timestamps as ISO 8601 UTC strings. A value the message left out, because it was NULL, is `null`. The names differ per check. | diff --git a/docs/scan.gif b/docs/scan.gif index 78a4d02..fb70d3c 100644 Binary files a/docs/scan.gif and b/docs/scan.gif differ diff --git a/src/Pgcheckup.Checks.Generator/CheckCompiler.cs b/src/Pgcheckup.Checks.Generator/CheckCompiler.cs index 0d96d88..7dfcb4d 100644 --- a/src/Pgcheckup.Checks.Generator/CheckCompiler.cs +++ b/src/Pgcheckup.Checks.Generator/CheckCompiler.cs @@ -129,8 +129,11 @@ public static class CheckCompiler /// The allowed values of severity, from most to least severe. public static readonly string[] Severities = ["critical", "warning", "info"]; - /// The predefined roles a check may list under privileges. All are part of pg_monitor. - public static readonly string[] Privileges = ["pg_monitor", "pg_read_all_settings", "pg_read_all_stats", "pg_stat_scan_tables"]; + /// + /// What a check may list under privileges: the predefined roles in pg_monitor, and + /// select_on_sequences for reading sequence counters. + /// + public static readonly string[] Privileges = ["pg_monitor", "pg_read_all_settings", "pg_read_all_stats", "pg_stat_scan_tables", "select_on_sequences"]; /// The managed providers a check may list under skip_on. public static readonly string[] Providers = ["rds", "aurora", "cloudsql", "azure", "supabase", "neon"]; diff --git a/src/Pgcheckup.Checks.Generator/Frontmatter.cs b/src/Pgcheckup.Checks.Generator/Frontmatter.cs index 1f14165..b6a4572 100644 --- a/src/Pgcheckup.Checks.Generator/Frontmatter.cs +++ b/src/Pgcheckup.Checks.Generator/Frontmatter.cs @@ -228,13 +228,36 @@ private static string Unquote(string value) if (end > 0 && (after.Length == 0 || after[0] == '#')) { var inner = value.Substring(1, end - 1); - return quote == '"' - ? inner.Replace("\\\"", "\"").Replace("\\\\", "\\") - : inner.Replace("''", "'"); + return quote == '"' ? Unescape(inner) : inner.Replace("''", "'"); } } var comment = value.IndexOf(" #", System.StringComparison.Ordinal); return comment >= 0 ? value.Substring(0, comment).TrimEnd() : value; } + + // The escapes YAML gives double-quoted scalars that a template can use. + private static string Unescape(string text) + { + var result = new StringBuilder(text.Length); + for (var i = 0; i < text.Length; i++) + { + if (text[i] == '\\' && i + 1 < text.Length) + { + i++; + result.Append(text[i] switch + { + 'n' => '\n', + 't' => '\t', + _ => text[i], + }); + } + else + { + result.Append(text[i]); + } + } + + return result.ToString(); + } } diff --git a/src/Pgcheckup/Checks/Template.cs b/src/Pgcheckup/Checks/Template.cs index 163133a..b4b817a 100644 --- a/src/Pgcheckup/Checks/Template.cs +++ b/src/Pgcheckup/Checks/Template.cs @@ -42,6 +42,14 @@ public sealed class Template(IReadOnlyList parts) /// The template's parts in order. public IReadOnlyList Parts { get; } = parts; + /// The columns the template uses, in order of first use, including those inside sections. + public IReadOnlyList ValueNames { get; } = parts + .SelectMany(p => p is SectionPart section ? section.Parts : [p]) + .OfType() + .Select(v => v.Name) + .Distinct(StringComparer.Ordinal) + .ToList(); + /// Renders the template with one row of the check's query. /// The row, by column name. SQL NULL is . /// The text, with each section left out when a value in it is NULL. diff --git a/src/Pgcheckup/Checks/ValueText.cs b/src/Pgcheckup/Checks/ValueText.cs index 5d9ae33..78f829f 100644 --- a/src/Pgcheckup/Checks/ValueText.cs +++ b/src/Pgcheckup/Checks/ValueText.cs @@ -1,4 +1,5 @@ using System.Globalization; +using System.Text; namespace Pgcheckup.Checks; @@ -23,10 +24,39 @@ public static class ValueText DateTime time => time.ToUniversalTime().ToString("yyyy-MM-dd HH:mm", CultureInfo.InvariantCulture) + " UTC", bool flag => flag ? "on" : "off", IFormattable number => number.ToString(null, CultureInfo.InvariantCulture), - _ => value.ToString() ?? "", + _ => Printable(value.ToString() ?? ""), }, }; + /// Makes text from the database safe to print: control characters become \uXXXX. + /// Text such as an object name, which anyone who can create a table controls. + /// + /// The text with every control character escaped, including line breaks, so it can't move the + /// cursor, rewrite a terminal line, set the clipboard or break out of Markdown. + /// + public static string Printable(string text) + { + if (!text.Any(char.IsControl)) + { + return text; + } + + var printable = new StringBuilder(text.Length + 8); + foreach (var c in text) + { + if (char.IsControl(c)) + { + printable.Append(CultureInfo.InvariantCulture, $"\\u{(int)c:x4}"); + } + else + { + printable.Append(c); + } + } + + return printable.ToString(); + } + /// Prints a size with Postgres's units (1024-based, as in pg_size_pretty) and three significant digits. /// The size in bytes. /// Such as "512 bytes", "1.5 GB" or "48 GB". diff --git a/src/Pgcheckup/Cli/CatalogText.cs b/src/Pgcheckup/Cli/CatalogText.cs new file mode 100644 index 0000000..5f73aff --- /dev/null +++ b/src/Pgcheckup/Cli/CatalogText.cs @@ -0,0 +1,59 @@ +using Pgcheckup.Checks; +using Pgcheckup.Engine; + +namespace Pgcheckup.Cli; + +/// What pgcheckup list and pgcheckup explain print. +public static class CatalogText +{ + private const int LabelWidth = 13; + + /// Writes one line per check, under a header: id, severity, category, minimum Postgres version and title. + /// Where to write. + /// The checks, in the order to list them. + public static void WriteList(TextWriter output, IReadOnlyList checks) + { + var idWidth = Math.Max("check".Length, checks.Count == 0 ? 0 : checks.Max(c => c.Id.Length)) + 2; + output.WriteLine($"{"check".PadRight(idWidth)}{"severity",-10}{"category",-10}{"postgres",-10}title"); + foreach (var check in checks) + { + output.WriteLine($"{check.Id.PadRight(idWidth)}{Lower(check.Severity),-10}{check.Category,-10}{check.MinVersion + "+",-10}{check.Title}"); + } + } + + /// Writes a check's details, then its note: what breaks, how to fix it and where it was seen. + /// Where to write. + /// The check to explain. + /// Lines for providers and thresholds appear only when the check has some. + public static void WriteExplanation(TextWriter output, CheckDefinition check) + { + output.WriteLine($"{check.Id} · {check.Title}"); + output.WriteLine(); + WriteDetail(output, "Severity:", Lower(check.Severity)); + WriteDetail(output, "Category:", check.Category); + WriteDetail(output, "Postgres:", $"{check.MinVersion} or later"); + WriteDetail(output, "Needs:", check.Privileges.Count == 0 ? "no extra privileges" : string.Join(" and ", check.Privileges.Select(PrivilegeName))); + if (check.SkipOn.Count > 0) + { + WriteDetail(output, "Skipped on:", string.Join(", ", check.SkipOn.Select(ProviderName))); + } + + if (check.Thresholds.Count > 0) + { + WriteDetail(output, "Thresholds:", string.Join(", ", check.Thresholds.Select(t => $"{t.Name} = {t.Text}"))); + } + + output.WriteLine(); + output.WriteLine(check.Note); + } + + private static void WriteDetail(TextWriter output, string label, string value) => + output.WriteLine($"{label.PadRight(LabelWidth)}{value}"); + + private static string Lower(Severity severity) => severity.ToString().ToLowerInvariant(); + + private static string PrivilegeName(string privilege) => + privilege == Applicability.SelectOnSequences ? "SELECT on sequences" : privilege; + + private static string ProviderName(string id) => Provider.Known.FirstOrDefault(p => p.Id == id)?.Name ?? id; +} diff --git a/src/Pgcheckup/Cli/GrantScript.cs b/src/Pgcheckup/Cli/GrantScript.cs new file mode 100644 index 0000000..60c4cc7 --- /dev/null +++ b/src/Pgcheckup/Cli/GrantScript.cs @@ -0,0 +1,69 @@ +using System.Text; +using System.Text.RegularExpressions; + +namespace Pgcheckup.Cli; + +/// The SQL that pgcheckup grant prints for a least-privilege checkup role. +public static partial class GrantScript +{ + // Reserved words can't be role or database names without quotes (PostgreSQL docs, appendix C). + private static readonly HashSet Reserved = new(StringComparer.Ordinal) + { + "all", "analyse", "analyze", "and", "any", "array", "as", "asc", "asymmetric", "authorization", + "binary", "both", "case", "cast", "check", "collate", "collation", "column", "concurrently", + "constraint", "create", "cross", "current_catalog", "current_date", "current_role", + "current_schema", "current_time", "current_timestamp", "current_user", "default", "deferrable", + "desc", "distinct", "do", "else", "end", "except", "false", "fetch", "for", "foreign", "freeze", + "from", "full", "grant", "group", "having", "ilike", "in", "initially", "inner", "intersect", + "into", "is", "isnull", "join", "lateral", "leading", "left", "like", "limit", "localtime", + "localtimestamp", "natural", "not", "notnull", "null", "offset", "on", "only", "or", "order", + "outer", "overlaps", "placing", "primary", "references", "returning", "right", "select", + "session_user", "similar", "some", "symmetric", "system_user", "table", "tablesample", "then", + "to", "trailing", "true", "union", "unique", "user", "using", "variadic", "verbose", "when", + "where", "window", "with", + }; + + /// Builds the SQL. It is printed for a person to review and run, never run by pgcheckup. + /// The role to create, quoted when the name needs it. + /// The database the role may connect to, quoted when the name needs it. + /// + /// The role that owns the application's tables. Default privileges only cover sequences that + /// this role creates later. + /// + /// + /// SQL that creates the role with pg_monitor, CONNECT and a read-only default, then + /// the sequence grants that only integer-exhaustion needs. + /// + /// A name contains a control character. + public static string Build(string role, string database, string owner) + { + var r = Identifier(role); + var d = Identifier(database); + var o = Identifier(owner); + var sql = new StringBuilder(); + sql.Append("-- A least-privilege role for pgcheckup. Review it, then run it as a superuser\n"); + sql.Append("-- (rds_superuser on Amazon RDS, cloudsqlsuperuser on Cloud SQL).\n"); + sql.Append($"CREATE ROLE {r} LOGIN;\n"); + sql.Append($"-- Set its password with \\password {r}, or use your provider's IAM login.\n"); + sql.Append($"GRANT pg_monitor TO {r};\n"); + sql.Append($"GRANT CONNECT ON DATABASE {d} TO {r};\n"); + sql.Append("-- Every session of this role is read-only, even outside pgcheckup.\n"); + sql.Append($"ALTER ROLE {r} SET default_transaction_read_only = on;\n"); + sql.Append('\n'); + sql.Append("-- Only integer-exhaustion needs these. They show sequence counters, never table rows.\n"); + sql.Append($"-- Repeat them for each schema with sequences. The second covers sequences that {o},\n"); + sql.Append("-- the role that owns your tables, creates later; name another with --owner.\n"); + sql.Append($"GRANT SELECT ON ALL SEQUENCES IN SCHEMA public TO {r};\n"); + sql.Append($"ALTER DEFAULT PRIVILEGES FOR ROLE {o} IN SCHEMA public GRANT SELECT ON SEQUENCES TO {r};\n"); + return sql.ToString(); + } + + // A line break in a name would end the -- comment it appears in, and run the rest as SQL. + private static string Identifier(string name) => + name.Any(char.IsControl) ? throw new ArgumentException("A role or database name can't contain control characters.") + : PlainIdentifier().IsMatch(name) && !Reserved.Contains(name) ? name + : $"\"{name.Replace("\"", "\"\"")}\""; + + [GeneratedRegex("^[a-z_][a-z0-9_$]*$")] + private static partial Regex PlainIdentifier(); +} diff --git a/src/Pgcheckup/Cli/JsonReport.cs b/src/Pgcheckup/Cli/JsonReport.cs new file mode 100644 index 0000000..ef58b5d --- /dev/null +++ b/src/Pgcheckup/Cli/JsonReport.cs @@ -0,0 +1,105 @@ +using System.Globalization; +using System.Text.Json; +using System.Text.Json.Nodes; +using System.Text.Json.Serialization; +using Pgcheckup.Checks; +using Pgcheckup.Engine; + +namespace Pgcheckup.Cli; + +/// +/// The --format json report. Its shape is a contract, versioned by schema and +/// described in docs/json.md. +/// +public static class JsonReport +{ + /// The version of the JSON shape. Change it only through a decision in ROADMAP.md. + public const int Schema = 1; + + /// Writes the report as indented JSON. + /// What the scan found. + /// pgcheckup's own version, for the pgcheckup field. + /// The JSON text. + public static string Write(ScanReport report, string version) + { + var server = report.Server; + var document = new JsonDocumentModel( + Schema, + version, + new JsonServer(server.Database, server.Host, server.Version, server.Provider is { } p ? new JsonProvider(p.Id, p.Name) : null), + new JsonSummary( + report.Results.Count(r => r.Status == CheckStatus.Passed), + report.Results.Count(r => r.Worst == Severity.Critical), + report.Results.Count(r => r.Worst == Severity.Warning), + report.Results.Count(r => r.Worst == Severity.Info), + report.Results.Count(r => r.Status == CheckStatus.Errored), + report.Results.Count(r => r.Status == CheckStatus.Skipped)), + report.Results.Select(ToJson).ToList()); + return JsonSerializer.Serialize(document, JsonReportContext.Default.JsonDocumentModel); + } + + private static JsonCheck ToJson(CheckResult result) => new( + result.Check.Id, + result.Check.Title, + result.Check.Category, + result.Status switch + { + CheckStatus.Found => Lower(result.Worst!.Value), + _ => result.Status.ToString().ToLowerInvariant(), + }, + result.Reason, + result.Findings.Select(f => new JsonFinding(f.Subject, Lower(f.Severity), f.Message, f.Fix, Values(result.Check, f))).ToList()); + + // Only the columns the message uses: the facts behind the finding, not helpers such as a + // quoted name that exists for the fix. + private static JsonObject Values(CheckDefinition check, Finding finding) + { + var values = new JsonObject(); + foreach (var name in check.Message.ValueNames.Where(finding.Values.ContainsKey)) + { + values[name] = ToNode(finding.Values[name]); + } + + return values; + } + + private static JsonNode? ToNode(object? value) => value switch + { + null => null, + string text => JsonValue.Create(text), + bool flag => JsonValue.Create(flag), + short number => JsonValue.Create(number), + int number => JsonValue.Create(number), + long number => JsonValue.Create(number), + decimal number => JsonValue.Create(number), + double number => JsonValue.Create(number), + TimeSpan span => JsonValue.Create(Math.Round((decimal)span.TotalSeconds, 3)), + DateTime time => JsonValue.Create(time.ToUniversalTime().ToString("yyyy-MM-dd'T'HH:mm:ss'Z'", CultureInfo.InvariantCulture)), + _ => JsonValue.Create(Convert.ToString(value, CultureInfo.InvariantCulture)), + }; + + private static string Lower(Severity severity) => severity.ToString().ToLowerInvariant(); +} + +// The JSON shape; docs/json.md describes each field. +internal sealed record JsonDocumentModel(int Schema, string Pgcheckup, JsonServer Server, JsonSummary Summary, IReadOnlyList Checks); + +internal sealed record JsonServer(string Database, string Host, string Version, JsonProvider? Provider); + +internal sealed record JsonProvider(string Id, string Name); + +internal sealed record JsonSummary(int Passed, int Critical, int Warning, int Info, int Errored, int Skipped); + +internal sealed record JsonCheck( + string Id, + string Title, + string Category, + string Status, + [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] string? Reason, + IReadOnlyList Findings); + +internal sealed record JsonFinding(string Subject, string Severity, string Message, string Fix, JsonObject Values); + +[JsonSourceGenerationOptions(PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase, WriteIndented = true)] +[JsonSerializable(typeof(JsonDocumentModel))] +internal sealed partial class JsonReportContext : JsonSerializerContext; diff --git a/src/Pgcheckup/Cli/MarkdownReport.cs b/src/Pgcheckup/Cli/MarkdownReport.cs new file mode 100644 index 0000000..35379fd --- /dev/null +++ b/src/Pgcheckup/Cli/MarkdownReport.cs @@ -0,0 +1,83 @@ +using System.Text; +using Pgcheckup.Checks; +using Pgcheckup.Engine; + +namespace Pgcheckup.Cli; + +/// The --format markdown report, shaped for a pull request comment. +public static class MarkdownReport +{ + /// + /// Writes a heading, a summary line, a table of findings and errored checks, then each fix in + /// a code block. With nothing found or errored, only the heading and summary. + /// + /// What the scan found. + /// The Markdown text. + public static string Write(ScanReport report) + { + var server = report.Server; + var markdown = new StringBuilder(); + Line(markdown, $"## pgcheckup · {Cell(ValueText.Printable(server.Database))} on {Cell(ValueText.Printable(server.Host))}"); + Line(markdown); + var provider = server.Provider is { } managed ? $" · {managed.Name}" : ""; + Line(markdown, $"PostgreSQL {server.Version}{provider} · {ReportText.Summary(report)}"); + + var findings = ReportText.OrderedFindings(report).ToList(); + var errored = ReportText.Errored(report).ToList(); + if (findings.Count == 0 && errored.Count == 0) + { + return markdown.ToString(); + } + + Line(markdown); + Line(markdown, "| Severity | Check | Finding |"); + Line(markdown, "| --- | --- | --- |"); + foreach (var finding in findings) + { + var severity = finding.Severity.ToString(); + Line(markdown, $"| {(finding.Severity == Severity.Critical ? $"**{severity}**" : severity)} | `{finding.CheckId}` | {Cell(finding.Message)} |"); + } + + foreach (var result in errored) + { + Line(markdown, $"| Errored | `{result.Check.Id}` | {Cell(ReportText.Sentence(result.Reason))} |"); + } + + if (findings.Count > 0) + { + Line(markdown); + Line(markdown, "### Fixes"); + foreach (var finding in findings) + { + Line(markdown); + Line(markdown, $"**`{finding.CheckId}`** · {Cell(ValueText.Printable(finding.Subject))}"); + Line(markdown); + + // A fence longer than any run of backticks in the fix, so an object name can't close it. + var fence = new string('`', Math.Max(3, LongestBacktickRun(finding.Fix) + 1)); + Line(markdown, fence); + Line(markdown, finding.Fix); + Line(markdown, fence); + } + } + + return markdown.ToString(); + } + + // Written with \n on every platform, so the text is the same wherever it is produced. + private static void Line(StringBuilder markdown, string text = "") => markdown.Append(text).Append('\n'); + + private static string Cell(string text) => text.Replace("|", "\\|").Replace("\n", "
"); + + private static int LongestBacktickRun(string text) + { + int longest = 0, run = 0; + foreach (var c in text) + { + run = c == '`' ? run + 1 : 0; + longest = Math.Max(longest, run); + } + + return longest; + } +} diff --git a/src/Pgcheckup/Cli/PgcheckupCli.cs b/src/Pgcheckup/Cli/PgcheckupCli.cs index 07ac8c3..10c4ea4 100644 --- a/src/Pgcheckup/Cli/PgcheckupCli.cs +++ b/src/Pgcheckup/Cli/PgcheckupCli.cs @@ -1,4 +1,5 @@ using System.CommandLine; +using System.Reflection; using Npgsql; using Pgcheckup.Checks; using Pgcheckup.Engine; @@ -17,6 +18,9 @@ public static class PgcheckupCli /// Exit code 2: the scan couldn't run, whatever the reason. public const int CouldNotRun = 2; + private static string Version => + typeof(PgcheckupCli).Assembly.GetCustomAttribute()?.InformationalVersion ?? "unknown"; + /// Runs pgcheckup with every check compiled into the binary. /// The command-line arguments. /// Where the report and help go. @@ -57,10 +61,54 @@ public static async Task RunAsync( { var root = new RootCommand("Checks a PostgreSQL database for the problems that cause outages. Read-only, and safe to run on production."); - var list = new Command("list", "List every check."); - list.SetAction(_ => List(checks, output)); + var list = new Command("list", "List every check with its severity, category and minimum Postgres version."); + list.SetAction(_ => + { + CatalogText.WriteList(output, checks); + return Passed; + }); root.Subcommands.Add(list); + var checkId = new Argument("check") { Description = "The id of a check, as pgcheckup list shows it." }; + var explain = new Command("explain", "Explain a check: what breaks, how to fix it, and where it has happened."); + explain.Arguments.Add(checkId); + explain.SetAction(result => Explain(checks, result.GetValue(checkId)!, output, error)); + root.Subcommands.Add(explain); + + var role = new Option("--role") + { + Description = "The role to create.", + DefaultValueFactory = _ => "checkup", + }; + var database = new Option("--database") + { + Description = "The database the role may connect to.", + DefaultValueFactory = _ => "app", + }; + var owner = new Option("--owner") + { + Description = "The role that owns your tables, so the grant covers sequences it creates later.", + DefaultValueFactory = _ => "app", + }; + var grant = new Command("grant", "Print SQL for a least-privilege checkup role. pgcheckup never runs it."); + grant.Options.Add(role); + grant.Options.Add(database); + grant.Options.Add(owner); + grant.SetAction(result => + { + try + { + output.Write(GrantScript.Build(result.GetValue(role)!, result.GetValue(database)!, result.GetValue(owner)!)); + return Passed; + } + catch (ArgumentException problem) + { + error.WriteLine($"pgcheckup: {problem.Message}"); + return CouldNotRun; + } + }); + root.Subcommands.Add(grant); + var connection = new Argument("connection") { Description = "A postgres:// URL or a libpq key-value string. Without one, the PG* environment variables are used. Keep the password in PGPASSWORD or ~/.pgpass, not here.", @@ -72,12 +120,19 @@ public static async Task RunAsync( DefaultValueFactory = _ => "critical", }; failOn.AcceptOnlyFromAmong("critical", "warning", "info"); + var format = new Option("--format") + { + Description = "How to write the report: terminal, json (see docs/json.md) or markdown.", + DefaultValueFactory = _ => "terminal", + }; + format.AcceptOnlyFromAmong("terminal", "json", "markdown"); var scan = new Command("scan", "Scan a database and report what could take it down."); scan.Arguments.Add(connection); scan.Options.Add(failOn); + scan.Options.Add(format); scan.SetAction((result, token) => ScanAsync( - result.GetValue(connection), result.GetValue(failOn)!, checks, output, error, environment, outputRedirected, token)); + result.GetValue(connection), result.GetValue(failOn)!, result.GetValue(format)!, checks, output, error, environment, outputRedirected, token)); root.Subcommands.Add(scan); var parsed = root.Parse(args); @@ -108,20 +163,35 @@ public static async Task RunAsync( } } - private static int List(IReadOnlyList checks, TextWriter output) + // Files and pipes get unwrapped lines; so does a console whose width can't be read. + private static int? TerminalWidth() { - var width = checks.Max(c => c.Id.Length) + 2; - foreach (var check in checks) + try + { + return Console.WindowWidth > 0 ? Console.WindowWidth : null; + } + catch (IOException) { - output.WriteLine($"{check.Id.PadRight(width)}{check.Severity.ToString().ToLowerInvariant(),-10}{check.Title}"); + return null; } + } + private static int Explain(IReadOnlyList checks, string id, TextWriter output, TextWriter error) + { + if (checks.FirstOrDefault(c => c.Id == id) is not { } check) + { + error.WriteLine($"pgcheckup: there is no check named {id}. Run pgcheckup list to see them."); + return CouldNotRun; + } + + CatalogText.WriteExplanation(output, check); return Passed; } private static async Task ScanAsync( string? input, string failOn, + string format, IReadOnlyList checks, TextWriter output, TextWriter error, @@ -159,13 +229,24 @@ private static async Task ScanAsync( { report = await Scanner.ScanAsync(session, host, checks, cancellationToken); } - catch (Exception problem) when (problem is CheckFailedException or NpgsqlException) + catch (NpgsqlException problem) { - error.WriteLine($"pgcheckup: {problem.Message}"); + error.WriteLine($"pgcheckup: couldn't read the server's version and privileges: {problem.Message}"); return CouldNotRun; } - TerminalReport.Write(output, report, TerminalReport.UseColor(outputRedirected, environment)); + switch (format) + { + case "json": + output.WriteLine(JsonReport.Write(report, Version)); + break; + case "markdown": + output.Write(MarkdownReport.Write(report)); + break; + default: + TerminalReport.Write(output, report, TerminalReport.UseColor(outputRedirected, environment), outputRedirected ? null : TerminalWidth()); + break; + } var threshold = failOn switch { @@ -173,7 +254,13 @@ private static async Task ScanAsync( "warning" => Severity.Warning, _ => Severity.Critical, }; - return report.Results.Any(r => r.Worst >= threshold) ? FindingsReachedFailOn : Passed; + // A finding outranks an error: it is the more useful signal, and an error still fails CI. + if (report.Results.Any(r => r.Worst >= threshold)) + { + return FindingsReachedFailOn; + } + + return report.Results.Any(r => r.Status == CheckStatus.Errored) ? CouldNotRun : Passed; } } } diff --git a/src/Pgcheckup/Cli/ReportText.cs b/src/Pgcheckup/Cli/ReportText.cs new file mode 100644 index 0000000..2251d1d --- /dev/null +++ b/src/Pgcheckup/Cli/ReportText.cs @@ -0,0 +1,77 @@ +using Pgcheckup.Checks; +using Pgcheckup.Engine; + +namespace Pgcheckup.Cli; + +/// Wording that the terminal and Markdown reports share, so they never disagree. +public static class ReportText +{ + /// + /// Counts each check once at its worst severity, then errored checks, then names each skipped + /// check with its reason. + /// + /// What the scan found. + /// Such as "11 passed · 1 critical · 1 warning · 1 skipped (wal-archiving-failing: managed by Amazon RDS)". + public static string Summary(ScanReport report) + { + var parts = new List { $"{report.Results.Count(r => r.Status == CheckStatus.Passed)} passed" }; + var critical = report.Results.Count(r => r.Worst == Severity.Critical); + var warning = report.Results.Count(r => r.Worst == Severity.Warning); + var info = report.Results.Count(r => r.Worst == Severity.Info); + var errored = report.Results.Count(r => r.Status == CheckStatus.Errored); + var skipped = report.Results.Where(r => r.Status == CheckStatus.Skipped).ToList(); + + if (critical > 0) + { + parts.Add($"{critical} critical"); + } + + if (warning > 0) + { + parts.Add(warning == 1 ? "1 warning" : $"{warning} warnings"); + } + + if (info > 0) + { + parts.Add($"{info} info"); + } + + if (errored > 0) + { + parts.Add($"{errored} errored"); + } + + if (skipped.Count > 0) + { + parts.Add($"{skipped.Count} skipped ({string.Join(", ", skipped.Select(r => $"{r.Check.Id}: {ValueText.Printable(r.Reason ?? "")}"))})"); + } + + return string.Join(" · ", parts); + } + + /// Every finding across all checks, most severe first, then by check id, then in the check's order. + /// What the scan found. + /// The findings in report order. + public static IEnumerable OrderedFindings(ScanReport report) => report.Results + .SelectMany(r => r.Findings) + .Select((finding, order) => (finding, order)) + .OrderByDescending(f => f.finding.Severity) + .ThenBy(f => f.finding.CheckId, StringComparer.Ordinal) + .ThenBy(f => f.order) + .Select(f => f.finding); + + /// The errored checks, ordered by id. + /// What the scan found. + /// The errored results. + public static IEnumerable Errored(ScanReport report) => + report.Results.Where(r => r.Status == CheckStatus.Errored).OrderBy(r => r.Check.Id, StringComparer.Ordinal); + + /// Turns a reason such as "timed out after 5 s" into a sentence: "Timed out after 5 s." + /// A lowercase reason, or . A server error can quote object names in it. + /// The reason capitalized, ending in a full stop, and safe to print. + public static string Sentence(string? reason) + { + var text = ValueText.Printable(string.IsNullOrEmpty(reason) ? "it failed" : reason); + return char.ToUpperInvariant(text[0]) + text[1..] + (text.EndsWith('.') ? "" : "."); + } +} diff --git a/src/Pgcheckup/Cli/TerminalReport.cs b/src/Pgcheckup/Cli/TerminalReport.cs index fb6ad01..1724107 100644 --- a/src/Pgcheckup/Cli/TerminalReport.cs +++ b/src/Pgcheckup/Cli/TerminalReport.cs @@ -1,3 +1,4 @@ +using System.Text; using Pgcheckup.Checks; using Pgcheckup.Engine; @@ -22,85 +23,93 @@ public static bool UseColor(bool outputRedirected, IReadOnlyDictionary - /// Writes the header, then each finding with its fix, then a summary that counts each check - /// once at its worst severity. + /// Writes the header, then each finding with its fix, then each errored check, then a + /// summary that counts each check once at its worst severity and names skipped checks. /// /// Where to write. /// What the scan found. - /// Whether to color the severity words. The words are always there. + /// Whether to color the status words. The words are always there. + /// + /// The terminal's width, to wrap messages at, or not to wrap. Fixes are + /// never wrapped: they are SQL to copy, and a line break inside a quoted name would change it. + /// /// Findings are ordered by severity, most severe first, then by check id. - public static void Write(TextWriter output, ScanReport report, bool color) + public static void Write(TextWriter output, ScanReport report, bool color, int? width = null) { var server = report.Server; - output.WriteLine($"pgcheckup · {server.Database} on {server.Host} · PostgreSQL {server.Version}"); + var provider = server.Provider is { } managed ? $" · {managed.Name}" : ""; + output.WriteLine($"pgcheckup · {ValueText.Printable(server.Database)} on {ValueText.Printable(server.Host)} · PostgreSQL {server.Version}{provider}"); output.WriteLine(); - var findings = report.Results - .SelectMany(r => r.Findings) - .Select((finding, order) => (finding, order)) - .OrderByDescending(f => f.finding.Severity) - .ThenBy(f => f.finding.CheckId, StringComparer.Ordinal) - .ThenBy(f => f.order) - .Select(f => f.finding); + foreach (var finding in ReportText.OrderedFindings(report)) + { + WriteLabel(output, finding.Severity.ToString().ToUpperInvariant(), SeverityColor(finding.Severity), finding.CheckId, color); + WriteIndented(output, finding.Message, new string(' ', Indent), new string(' ', Indent), width); + WriteIndented(output, finding.Fix, new string(' ', Indent) + FixLabel, new string(' ', Indent + FixLabel.Length), null); + output.WriteLine(); + } - foreach (var finding in findings) + foreach (var errored in ReportText.Errored(report)) { - var label = finding.Severity.ToString().ToUpperInvariant(); - output.WriteLine($"{Paint(label, finding.Severity, color)}{new string(' ', Indent - label.Length)}{finding.CheckId}"); - WriteIndented(output, finding.Message, new string(' ', Indent), new string(' ', Indent)); - WriteIndented(output, finding.Fix, new string(' ', Indent) + FixLabel, new string(' ', Indent + FixLabel.Length)); + WriteLabel(output, "ERRORED", ErroredColor, errored.Check.Id, color); + WriteIndented(output, ReportText.Sentence(errored.Reason), new string(' ', Indent), new string(' ', Indent), width); output.WriteLine(); } - output.WriteLine(Summary(report)); + output.WriteLine(ReportText.Summary(report)); + } + + private static void WriteLabel(TextWriter output, string label, string colorCode, string checkId, bool color) + { + var painted = color ? $"\u001b[{colorCode}m{label}\u001b[0m" : label; + output.WriteLine($"{painted}{new string(' ', Indent - label.Length)}{checkId}"); } - private static void WriteIndented(TextWriter output, string text, string first, string rest) + private static void WriteIndented(TextWriter output, string text, string first, string rest, int? width) { - var lines = text.Split('\n'); - for (var i = 0; i < lines.Length; i++) + var lines = text.Split('\n').SelectMany(line => width is { } w ? Wrap(line, w - rest.Length) : [line]).ToList(); + for (var i = 0; i < lines.Count; i++) { output.WriteLine((i == 0 ? first : rest) + lines[i]); } } - private static string Summary(ScanReport report) + // Breaks at spaces. A word longer than the line stays whole, and a very narrow terminal gets + // no wrapping rather than a column of single words. + private static IEnumerable Wrap(string line, int available) { - var parts = new List { $"{report.Results.Count(r => r.Worst == null)} passed" }; - var critical = report.Results.Count(r => r.Worst == Severity.Critical); - var warning = report.Results.Count(r => r.Worst == Severity.Warning); - var info = report.Results.Count(r => r.Worst == Severity.Info); - if (critical > 0) + if (available < 20 || line.Length <= available) { - parts.Add($"{critical} critical"); + yield return line; + yield break; } - if (warning > 0) + var current = new StringBuilder(); + foreach (var word in line.Split(' ')) { - parts.Add(warning == 1 ? "1 warning" : $"{warning} warnings"); - } + if (current.Length > 0 && current.Length + 1 + word.Length > available) + { + yield return current.ToString(); + current.Clear(); + } - if (info > 0) - { - parts.Add($"{info} info"); + if (current.Length > 0) + { + current.Append(' '); + } + + current.Append(word); } - return string.Join(" · ", parts); + yield return current.ToString(); } - private static string Paint(string label, Severity severity, bool color) - { - if (!color) - { - return label; - } + private const string ErroredColor = "1;35"; - var code = severity switch - { - Severity.Critical => "1;31", - Severity.Warning => "1;33", - _ => "1;34", - }; - return $"\u001b[{code}m{label}\u001b[0m"; - } + private static string SeverityColor(Severity severity) => severity switch + { + Severity.Critical => "1;31", + Severity.Warning => "1;33", + _ => "1;34", + }; } diff --git a/src/Pgcheckup/Engine/Applicability.cs b/src/Pgcheckup/Engine/Applicability.cs new file mode 100644 index 0000000..e4bb057 --- /dev/null +++ b/src/Pgcheckup/Engine/Applicability.cs @@ -0,0 +1,38 @@ +using Pgcheckup.Checks; + +namespace Pgcheckup.Engine; + +/// Decides whether a check can run on a server, before it runs. +public static class Applicability +{ + /// The privilege a check declares when it reads sequence counters. + public const string SelectOnSequences = "select_on_sequences"; + + /// Says why a check can't run here, if it can't. + /// The check. + /// The server and role. + /// + /// when the check can run. Otherwise one reason, checked in this order: + /// the Postgres version, the managed provider, then the role's privileges. + /// + public static string? SkipReason(CheckDefinition check, ServerContext context) + { + if (context.Major < check.MinVersion) + { + return $"needs Postgres {check.MinVersion} or later"; + } + + if (context.Provider is { } provider && check.SkipOn.Contains(provider.Id)) + { + return $"managed by {provider.Name}"; + } + + var missing = check.Privileges.Where(p => !context.Privileges.Contains(p)).ToList(); + if (missing.Contains(SelectOnSequences)) + { + return "can't read sequence counters; see pgcheckup grant"; + } + + return missing.Count > 0 ? $"needs {string.Join(" and ", missing)}" : null; + } +} diff --git a/src/Pgcheckup/Engine/Scanner.cs b/src/Pgcheckup/Engine/Scanner.cs index 9df65b0..75caf7a 100644 --- a/src/Pgcheckup/Engine/Scanner.cs +++ b/src/Pgcheckup/Engine/Scanner.cs @@ -1,77 +1,86 @@ -using System.Globalization; +using Npgsql; using Pgcheckup.Checks; namespace Pgcheckup.Engine; -/// What the report's header says about the server. -/// The database scanned. -/// The host as given, never the full connection string. -/// The Postgres version, such as "17.6". -public sealed record ServerInfo(string Database, string Host, string Version); +/// How a check's run ended. +public enum CheckStatus +{ + /// It ran and found nothing. + Passed, + + /// It ran and found at least one problem. + Found, + + /// It didn't run here, for the reason given. A skip never fails a scan. + Skipped, + + /// It ran and failed, for the reason given. The rest of the scan still ran. + Errored, +} -/// One check's findings. -/// The check that ran. -/// What it found. Empty when it passed. -public sealed record CheckResult(CheckDefinition Check, IReadOnlyList Findings) +/// One check's outcome. +/// The check. +/// How its run ended. +/// What it found. Empty unless is . +/// Why it was skipped or errored, or when it ran. +public sealed record CheckResult(CheckDefinition Check, CheckStatus Status, IReadOnlyList Findings, string? Reason = null) { - /// The most severe finding's severity, or when the check passed. + /// The most severe finding's severity, or when there are no findings. public Severity? Worst => Findings.Count == 0 ? null : Findings.Max(f => f.Severity); } /// Everything a scan found. -/// The server scanned. -/// Each check's findings, in the order the checks ran. -public sealed record ScanReport(ServerInfo Server, IReadOnlyList Results); - -/// A check that couldn't run. In M0 this ends the scan; M1 reports it as errored and goes on. -/// The check that failed. -/// What went wrong. -public sealed class CheckFailedException(string checkId, Exception inner) - : Exception($"{checkId} couldn't run: {inner.Message}", inner) -{ - /// The check that failed. - public string CheckId { get; } = checkId; -} +/// The server scanned, and the role's privileges. +/// Each check's outcome, in the order the checks were given. +public sealed record ScanReport(ServerContext Server, IReadOnlyList Results); /// Runs checks against one database. public static class Scanner { - /// Reads the server's version and database, then runs each check in turn. + /// + /// Reads the server context, then runs every check that applies. A check that fails is + /// reported as errored, and the others still run. + /// /// The guarded session to query through. /// The host to name in the report. - /// The checks to run, in order. + /// The checks, in the order to run and report them. /// Cancels the scan. - /// The server and each check's findings. - /// A check failed for any reason. The scan stops there. - /// The server's version or database couldn't be read. + /// The server context and every check's outcome. + /// The server context couldn't be read, so no check ran. + /// The scan was cancelled. public static async Task ScanAsync( ReadOnlySession session, string host, IReadOnlyList checks, CancellationToken cancellationToken) { - var row = (await session.QueryAsync( - "SELECT current_database() AS database, current_setting('server_version_num')::int AS version", - [], - cancellationToken)).Single(); - - // server_version_num is major * 10000 + minor from Postgres 10 on. - var version = (int)row["version"]!; - var server = new ServerInfo( - (string)row["database"]!, - host, - string.Create(CultureInfo.InvariantCulture, $"{version / 10000}.{version % 10000}")); - + var context = await ServerContext.ReadAsync(session, host, cancellationToken); var results = new List(); foreach (var check in checks) { + if (Applicability.SkipReason(check, context) is { } reason) + { + results.Add(new CheckResult(check, CheckStatus.Skipped, [], reason)); + continue; + } + try { - results.Add(new CheckResult(check, await CheckRunner.RunAsync(session, check, cancellationToken))); + var findings = await CheckRunner.RunAsync(session, check, cancellationToken); + results.Add(new CheckResult(check, findings.Count == 0 ? CheckStatus.Passed : CheckStatus.Found, findings)); } catch (Exception error) when (error is not OperationCanceledException) { - throw new CheckFailedException(check.Id, error); + results.Add(new CheckResult(check, CheckStatus.Errored, [], Describe(error))); } } - return new ScanReport(server, results); + return new ScanReport(context, results); } + + private static string Describe(Exception error) => error switch + { + PostgresException { SqlState: PostgresErrorCodes.QueryCanceled } => "timed out after 5 s", + PostgresException { SqlState: PostgresErrorCodes.LockNotAvailable } => "waited over 1 s for a lock", + PostgresException postgres => $"{postgres.SqlState}: {postgres.MessageText}", + _ => error.Message, + }; } diff --git a/src/Pgcheckup/Engine/ServerContext.cs b/src/Pgcheckup/Engine/ServerContext.cs new file mode 100644 index 0000000..0dc6f42 --- /dev/null +++ b/src/Pgcheckup/Engine/ServerContext.cs @@ -0,0 +1,87 @@ +using System.Globalization; + +namespace Pgcheckup.Engine; + +/// A managed Postgres service, detected from SQL. +/// The id that check.md lists under skip_on, such as rds. +/// The name the report prints, such as "Amazon RDS". +public sealed record Provider(string Id, string Name) +{ + /// Every provider pgcheckup detects, in detection order. + public static IReadOnlyList Known { get; } = + [ + new("aurora", "Amazon Aurora"), + new("rds", "Amazon RDS"), + new("cloudsql", "Google Cloud SQL"), + new("azure", "Azure Database for PostgreSQL"), + new("supabase", "Supabase"), + new("neon", "Neon"), + ]; +} + +/// What pgcheckup knows about the server before any check runs. +/// The database scanned. +/// The host as given, never the full connection string. +/// The server's server_version_num, such as 170006. +/// The managed service, or when none was detected. +/// +/// The privileges a check can declare that this role has: the predefined roles it is a member of, +/// and select_on_sequences when it can read every sequence's counter. +/// +public sealed record ServerContext( + string Database, + string Host, + int VersionNumber, + Provider? Provider, + IReadOnlySet Privileges) +{ + private static readonly string[] PrivilegeColumns = + ["pg_monitor", "pg_read_all_settings", "pg_read_all_stats", "pg_stat_scan_tables", "select_on_sequences"]; + + // One statement, so it runs in one guarded transaction. Provider signals are roles, a function + // or settings that each service creates; a self-managed server has none of them. + private const string Sql = """ + SELECT current_database() AS database, + current_setting('server_version_num')::int AS version, + pg_has_role(current_user, 'pg_monitor', 'USAGE') AS pg_monitor, + pg_has_role(current_user, 'pg_read_all_settings', 'USAGE') AS pg_read_all_settings, + pg_has_role(current_user, 'pg_read_all_stats', 'USAGE') AS pg_read_all_stats, + pg_has_role(current_user, 'pg_stat_scan_tables', 'USAGE') AS pg_stat_scan_tables, + -- Starting from pg_sequence, because Postgres may test the privilege before a + -- relkind filter and fail on a relation that isn't a sequence. + NOT EXISTS ( + SELECT FROM pg_sequence AS s JOIN pg_class AS c ON c.oid = s.seqrelid + WHERE c.relpersistence <> 't' AND NOT has_sequence_privilege(s.seqrelid, 'SELECT,USAGE') + ) AS select_on_sequences, + EXISTS (SELECT FROM pg_proc WHERE proname = 'aurora_version') AS aurora, + EXISTS (SELECT FROM pg_roles WHERE rolname = 'rds_superuser') AS rds, + EXISTS (SELECT FROM pg_roles WHERE rolname = 'cloudsqlsuperuser') AS cloudsql, + EXISTS (SELECT FROM pg_roles WHERE rolname = 'azure_pg_admin') AS azure, + EXISTS (SELECT FROM pg_roles WHERE rolname = 'supabase_admin') AS supabase, + current_setting('neon.tenant_id', true) IS NOT NULL AS neon + """; + + /// The major version, such as 17. + public int Major => VersionNumber / 10000; + + /// The version as people write it, such as "17.6". + public string Version => string.Create(CultureInfo.InvariantCulture, $"{Major}.{VersionNumber % 10000}"); + + /// Reads the server's version, provider and the role's privileges in one query. + /// The guarded session to query through. + /// The host to name in the report. + /// Cancels the query. + /// The context every check's applicability is decided from. + /// The query failed. + public static async Task ReadAsync(ReadOnlySession session, string host, CancellationToken cancellationToken) + { + var row = (await session.QueryAsync(Sql, [], cancellationToken)).Single(); + return new ServerContext( + (string)row["database"]!, + host, + (int)row["version"]!, + // Aurora comes first in Provider.Known because it also has rds_superuser. + Provider.Known.FirstOrDefault(p => (bool)row[p.Id]!), + PrivilegeColumns.Where(p => (bool)row[p]!).ToHashSet(StringComparer.Ordinal)); + } +} diff --git a/tests/Pgcheckup.Tests/Checks/CatalogTests.cs b/tests/Pgcheckup.Tests/Checks/CatalogTests.cs new file mode 100644 index 0000000..c7f2630 --- /dev/null +++ b/tests/Pgcheckup.Tests/Checks/CatalogTests.cs @@ -0,0 +1,23 @@ +using Pgcheckup.Checks; +using Pgcheckup.Engine; +using Pgcheckup.Tests.Postgres; + +namespace Pgcheckup.Tests.Checks; + +public class CatalogTests(PostgresServerFixture postgres) : IClassFixture +{ + // M1's "Done when": the least-privilege role gets a clean scan, where every check runs or + // says why it was skipped, and none errors. + [Fact] + public async Task A_pg_monitor_role_runs_or_skips_every_check_without_errors() + { + var cancel = TestContext.Current.CancellationToken; + await using var session = await ReadOnlySession.OpenAsync(postgres.Server.Checkup, cancel); + + var report = await Scanner.ScanAsync(session, "db.example.com", CheckCatalog.All, cancel); + + Assert.Equal(CheckCatalog.All.Count, report.Results.Count); + Assert.All(report.Results, r => Assert.True(r.Status != CheckStatus.Errored, $"{r.Check.Id} errored: {r.Reason}")); + Assert.All(report.Results.Where(r => r.Status == CheckStatus.Skipped), r => Assert.False(string.IsNullOrEmpty(r.Reason))); + } +} diff --git a/tests/Pgcheckup.Tests/Checks/CheckCompilerTests.cs b/tests/Pgcheckup.Tests/Checks/CheckCompilerTests.cs index c61d1f8..ee75297 100644 --- a/tests/Pgcheckup.Tests/Checks/CheckCompilerTests.cs +++ b/tests/Pgcheckup.Tests/Checks/CheckCompilerTests.cs @@ -107,6 +107,18 @@ public void Reads_quoted_scalars_and_ignores_comments() Assert.Equal("wal", check.Category); } + [Fact] + public void Reads_escaped_line_breaks_in_double_quoted_scalars_as_yaml_does() + { + var frontmatter = ValidFrontmatter.Replace(""" + fix: | + Do this: + SELECT 1; + """, "fix: \"Do this:\\nSELECT 1;\""); + + Assert.Equal("'Do this:\nSELECT 1;'", TemplateText.Describe(Compile(Markdown(frontmatter)).Check!.Fix)); + } + [Fact] public void Requires_frontmatter() { diff --git a/tests/Pgcheckup.Tests/Checks/FixtureTests.cs b/tests/Pgcheckup.Tests/Checks/FixtureTests.cs index afe4217..9e20ba9 100644 --- a/tests/Pgcheckup.Tests/Checks/FixtureTests.cs +++ b/tests/Pgcheckup.Tests/Checks/FixtureTests.cs @@ -5,16 +5,22 @@ namespace Pgcheckup.Tests.Checks; -// Every check runs against its own fixtures on a fresh Postgres, as the checkup role, through -// the same session and runner as a scan. `fires` must produce a finding and `healthy` must not. +// Every check runs against its own fixtures on a fresh Postgres, through the same session and +// runner as a scan. `fires` must produce a finding and `healthy` must not. public class FixtureTests { private static CancellationToken Cancel => TestContext.Current.CancellationToken; + // PGCHECKUP_TEST_CHECK= runs one check's fixtures, which is quicker while writing a check. + private static IEnumerable Selected => + Environment.GetEnvironmentVariable("PGCHECKUP_TEST_CHECK") is { Length: > 0 } id + ? CheckCatalog.All.Where(c => c.Id == id) + : CheckCatalog.All; + public static TheoryData Fixtures() { var data = new TheoryData(); - foreach (var check in CheckCatalog.All) + foreach (var check in Selected) { data.Add(check.Id, "fires"); data.Add(check.Id, "healthy"); @@ -23,9 +29,52 @@ public static TheoryData Fixtures() return data; } + public static TheoryData Checks() => new(Selected.Select(c => c.Id)); + [Theory] [MemberData(nameof(Fixtures))] public async Task Fires_on_its_fires_fixture_and_stays_quiet_on_healthy(string checkId, string fixture) + { + await using var prepared = await PrepareAsync(checkId, fixture); + + var findings = await RunAsync(prepared, prepared.Server.Checkup); + // PGCHECKUP_TEST_FINDINGS= collects what each fixture renders, for reviewing a check's wording. + if (Environment.GetEnvironmentVariable("PGCHECKUP_TEST_FINDINGS") is { Length: > 0 } path) + { + await File.AppendAllLinesAsync( + path, findings.Select(f => $"[{PostgresServer.Version} {fixture}] {f.Severity} {f.CheckId}: {f.Message} | Fix: {f.Fix.Replace('\n', ' ')}"), Cancel); + } + + if (fixture == "fires") + { + Assert.NotEmpty(findings); + } + else + { + Assert.Empty(findings); + } + } + + // A check can pass while seeing nothing: without pg_read_all_stats, for example, + // pg_stat_activity hides other users' sessions. So the declared privileges must be enough. + [Theory] + [MemberData(nameof(Checks))] + public async Task Fires_as_a_role_with_only_its_declared_privileges(string checkId) + { + await using var prepared = await PrepareAsync(checkId, "fires"); + var grants = prepared.Check.Privileges.Select(p => p == Applicability.SelectOnSequences + ? "GRANT SELECT ON ALL SEQUENCES IN SCHEMA public TO declared" + : $"GRANT {p} TO declared"); + await prepared.Server.ExecuteAsSuperuserAsync(Cancel, ["CREATE ROLE declared LOGIN PASSWORD 'declared'", .. grants]); + + var declared = prepared.Server.Checkup; + declared.Username = "declared"; + declared.Password = "declared"; + + Assert.NotEmpty(await RunAsync(prepared, declared)); + } + + private static async Task PrepareAsync(string checkId, string fixture) { var check = CheckCatalog.All.Single(c => c.Id == checkId); if (int.Parse(PostgresServer.Version) < check.MinVersion) @@ -34,26 +83,38 @@ public async Task Fires_on_its_fires_fixture_and_stays_quiet_on_healthy(string c } var script = FixtureScript.Load(checkId, fixture); - await using var server = await PostgresServer.StartAsync(Cancel); + var server = await PostgresServer.StartAsync(script.ServerSettings, Cancel); // Stays open until the check has run, so a fixture can hold a transaction or lock open. - await using var setup = await server.OpenSuperuserAsync(Cancel); + var setup = await server.OpenSuperuserAsync(Cancel); foreach (var statement in script.Statements) { - await using var command = new NpgsqlCommand(statement, setup); - await command.ExecuteNonQueryAsync(Cancel); + await using var command = new NpgsqlCommand(statement.Sql, setup); + if (!statement.MayFail) + { + await command.ExecuteNonQueryAsync(Cancel); + continue; + } + + // The error is the fixture's point, so a statement that succeeds means the fixture is broken. + await Assert.ThrowsAsync(() => command.ExecuteNonQueryAsync(Cancel)); } - await using var session = await ReadOnlySession.OpenAsync(server.Checkup, Cancel); - var findings = await CheckRunner.RunAsync(session, script.Apply(check), Cancel); + return new PreparedFixture(server, setup, script.Apply(check)); + } - if (fixture == "fires") - { - Assert.NotEmpty(findings); - } - else + private static async Task> RunAsync(PreparedFixture prepared, NpgsqlConnectionStringBuilder role) + { + await using var session = await ReadOnlySession.OpenAsync(role, Cancel); + return await CheckRunner.RunAsync(session, prepared.Check, Cancel); + } + + private sealed record PreparedFixture(PostgresServer Server, NpgsqlConnection Setup, CheckDefinition Check) : IAsyncDisposable + { + public async ValueTask DisposeAsync() { - Assert.Empty(findings); + await Setup.DisposeAsync(); + await Server.DisposeAsync(); } } } diff --git a/tests/Pgcheckup.Tests/Checks/TemplateRenderTests.cs b/tests/Pgcheckup.Tests/Checks/TemplateRenderTests.cs index 64369b7..dc19d5c 100644 --- a/tests/Pgcheckup.Tests/Checks/TemplateRenderTests.cs +++ b/tests/Pgcheckup.Tests/Checks/TemplateRenderTests.cs @@ -79,6 +79,14 @@ public void Prints_other_values_plainly(object value, string expected) Assert.Equal(expected, RenderOne(value)); } + // Object names come from the database, and anyone who can create a table can put terminal + // escape sequences or line breaks in one. + [Fact] + public void Escapes_control_characters_in_text_values() + { + Assert.Equal("orders\\u001b[2K\\u000aDROP", RenderOne("orders\u001b[2K\nDROP")); + } + [Fact] public void Prints_decimals_and_timestamps_without_culture() { diff --git a/tests/Pgcheckup.Tests/Cli/CatalogCommandTests.cs b/tests/Pgcheckup.Tests/Cli/CatalogCommandTests.cs new file mode 100644 index 0000000..95d72ab --- /dev/null +++ b/tests/Pgcheckup.Tests/Cli/CatalogCommandTests.cs @@ -0,0 +1,120 @@ +using Pgcheckup.Checks; +using Pgcheckup.Cli; + +namespace Pgcheckup.Tests.Cli; + +public class CatalogCommandTests +{ + private static readonly CheckDefinition Sample = new( + "sample-check", "A sample check", "wal", Severity.Warning, 15, ["pg_monitor"], ["rds"], + [new Threshold("min_retained_wal", ThresholdKind.Bytes, 1_073_741_824m, "1GB")], + "SELECT 1", + new Template([]), + new Template([]), + "## What breaks\n\nDisks fill.\n\n## Fix\n\nDrop it.\n\n## Seen in\n\n- [Docs](https://www.postgresql.org/docs/current/)"); + + private static readonly CheckDefinition Other = Sample with + { + Id = "other-check", Title = "Another check", Category = "ids", Severity = Severity.Critical, MinVersion = 14, + Privileges = [], SkipOn = [], Thresholds = [], + }; + + private static async Task<(int ExitCode, string Output, string Error)> RunAsync(params string[] args) + { + var output = new StringWriter { NewLine = "\n" }; + var error = new StringWriter { NewLine = "\n" }; + var exitCode = await PgcheckupCli.RunAsync( + args, [Sample, Other], output, error, new Dictionary(), outputRedirected: true, TestContext.Current.CancellationToken); + return (exitCode, output.ToString(), error.ToString()); + } + + [Fact] + public async Task Lists_each_check_with_its_severity_category_and_minimum_version() + { + var (exitCode, output, _) = await RunAsync("list"); + + Assert.Equal(0, exitCode); + Assert.Equal( + """ + check severity category postgres title + sample-check warning wal 15+ A sample check + other-check critical ids 14+ Another check + + """.ReplaceLineEndings("\n"), + output); + } + + [Fact] + public async Task Explains_a_check_with_its_details_and_note() + { + var (exitCode, output, _) = await RunAsync("explain", "sample-check"); + + Assert.Equal(0, exitCode); + Assert.Equal( + """ + sample-check · A sample check + + Severity: warning + Category: wal + Postgres: 15 or later + Needs: pg_monitor + Skipped on: Amazon RDS + Thresholds: min_retained_wal = 1GB + + ## What breaks + + Disks fill. + + ## Fix + + Drop it. + + ## Seen in + + - [Docs](https://www.postgresql.org/docs/current/) + + """.ReplaceLineEndings("\n"), + output); + } + + [Fact] + public async Task Says_when_a_check_needs_nothing() + { + var (_, output, _) = await RunAsync("explain", "other-check"); + + Assert.Contains("Needs: no extra privileges\n", output); + Assert.DoesNotContain("Skipped on:", output); + Assert.DoesNotContain("Thresholds:", output); + } + + [Fact] + public async Task Grants_to_the_role_and_owner_given() + { + var (exitCode, output, _) = await RunAsync("grant", "--role", "scanner", "--database", "shop", "--owner", "shop_owner"); + + Assert.Equal(0, exitCode); + Assert.Contains("CREATE ROLE scanner LOGIN;", output); + Assert.Contains("GRANT CONNECT ON DATABASE shop TO scanner;", output); + Assert.Contains("ALTER DEFAULT PRIVILEGES FOR ROLE shop_owner IN SCHEMA public", output); + } + + [Fact] + public async Task Exits_2_when_grant_is_given_a_name_with_a_line_break() + { + var (exitCode, output, error) = await RunAsync("grant", "--role", "scanner\nDROP TABLE orders;"); + + Assert.Equal(2, exitCode); + Assert.Equal("", output); + Assert.Contains("control characters", error); + } + + [Fact] + public async Task Exits_2_for_a_check_that_does_not_exist() + { + var (exitCode, _, error) = await RunAsync("explain", "no-such-check"); + + Assert.Equal(2, exitCode); + Assert.Contains("no-such-check", error); + Assert.Contains("pgcheckup list", error); + } +} diff --git a/tests/Pgcheckup.Tests/Cli/CommandLineTests.cs b/tests/Pgcheckup.Tests/Cli/CommandLineTests.cs index 293b285..4265707 100644 --- a/tests/Pgcheckup.Tests/Cli/CommandLineTests.cs +++ b/tests/Pgcheckup.Tests/Cli/CommandLineTests.cs @@ -29,6 +29,7 @@ public async Task Lists_every_check() [Theory] [InlineData("scan", "--nope")] [InlineData("scan", "--fail-on", "sometimes")] + [InlineData("scan", "--format", "yaml")] [InlineData("frobnicate")] public async Task Exits_2_when_the_arguments_are_wrong(params string[] args) { @@ -60,11 +61,32 @@ public async Task Exits_2_when_it_cannot_connect() public class ScanCommandTests(InactiveSlotFixture postgres) : IClassFixture { + private static readonly CheckDefinition SlotCheck = CheckCatalog.All.Single(c => c.Id == "replication-slot-inactive"); + + // Npgsql can't read NaN into a decimal, so this check errors on every run. + private static readonly CheckDefinition BrokenCheck = new( + "nan-check", "NaN check", "wal", Severity.Critical, 14, [], [], [], + "SELECT 'a' AS subject, 'NaN'::numeric AS size", + new Template([new ValuePart("size", ValueFormat.Bytes)]), + new Template([new TextPart("nothing")]), + ""); + + // Pinning the checks keeps the summary stable as the catalog grows. + private async Task<(int ExitCode, string Output, string Error)> ScanAsync(CheckDefinition[] checks, params string[] options) + { + var output = new StringWriter { NewLine = "\n" }; + var error = new StringWriter { NewLine = "\n" }; + var exitCode = await PgcheckupCli.RunAsync( + ["scan", await postgres.CheckupUrlAsync(), .. options], checks, output, error, + new Dictionary(), outputRedirected: true, TestContext.Current.CancellationToken); + return (exitCode, output.ToString(), error.ToString()); + } + [Fact] public async Task Reports_a_warning_and_exits_0_below_the_default_fail_on() { var server = await postgres.ServerAsync(); - var (exitCode, output, error) = await CommandLineTests.RunAsync("scan", await postgres.CheckupUrlAsync()); + var (exitCode, output, error) = await ScanAsync([SlotCheck]); Assert.Equal("", error); Assert.Equal(0, exitCode); @@ -76,30 +98,46 @@ public async Task Reports_a_warning_and_exits_0_below_the_default_fail_on() } [Fact] - public async Task Exits_2_when_a_check_fails_in_a_way_nobody_planned_for() + public async Task Exits_1_when_a_finding_reaches_fail_on() { - // Npgsql can't read NaN into a decimal, so the runner throws something no catch expects. - var check = new CheckDefinition( - "nan-check", "NaN check", "wal", Severity.Critical, 14, [], [], [], - "SELECT 'a' AS subject, 'NaN'::numeric AS size", - new Template([new ValuePart("size", ValueFormat.Bytes)]), - new Template([new TextPart("nothing")]), - ""); - var output = new StringWriter(); - var error = new StringWriter(); + Assert.Equal(1, (await ScanAsync([SlotCheck], "--fail-on", "warning")).ExitCode); + } - var exitCode = await PgcheckupCli.RunAsync( - ["scan", await postgres.CheckupUrlAsync()], [check], output, error, new Dictionary(), outputRedirected: true, TestContext.Current.CancellationToken); + [Fact] + public async Task Writes_json_with_the_same_exit_codes() + { + var (exitCode, output, _) = await ScanAsync([SlotCheck], "--format", "json", "--fail-on", "warning"); - Assert.Equal(2, exitCode); - Assert.StartsWith("pgcheckup: ", error.ToString()); + Assert.Equal(1, exitCode); + using var json = System.Text.Json.JsonDocument.Parse(output); + Assert.Equal(1, json.RootElement.GetProperty("schema").GetInt32()); + Assert.Equal("warning", json.RootElement.GetProperty("checks")[0].GetProperty("status").GetString()); } [Fact] - public async Task Exits_1_when_a_finding_reaches_fail_on() + public async Task Writes_markdown() { - var (exitCode, _, _) = await CommandLineTests.RunAsync("scan", await postgres.CheckupUrlAsync(), "--fail-on", "warning"); + var (exitCode, output, _) = await ScanAsync([SlotCheck], "--format", "markdown"); - Assert.Equal(1, exitCode); + Assert.Equal(0, exitCode); + Assert.StartsWith("## pgcheckup · app on ", output); + Assert.Contains("| Warning | `replication-slot-inactive` |", output); + } + + [Fact] + public async Task Exits_2_when_a_check_errored_and_no_finding_reached_fail_on() + { + var (exitCode, output, error) = await ScanAsync([BrokenCheck, SlotCheck]); + + Assert.Equal(2, exitCode); + Assert.Equal("", error); + Assert.Contains("ERRORED nan-check", output); + Assert.Contains("WARNING replication-slot-inactive", output); + } + + [Fact] + public async Task Exits_1_when_a_finding_reaches_fail_on_even_if_a_check_errored() + { + Assert.Equal(1, (await ScanAsync([BrokenCheck, SlotCheck], "--fail-on", "warning")).ExitCode); } } diff --git a/tests/Pgcheckup.Tests/Cli/GrantTests.cs b/tests/Pgcheckup.Tests/Cli/GrantTests.cs new file mode 100644 index 0000000..afb2d44 --- /dev/null +++ b/tests/Pgcheckup.Tests/Cli/GrantTests.cs @@ -0,0 +1,87 @@ +using Npgsql; +using Pgcheckup.Checks; +using Pgcheckup.Cli; +using Pgcheckup.Engine; +using Pgcheckup.Tests.Postgres; + +namespace Pgcheckup.Tests.Cli; + +public class GrantTests(PostgresServerFixture postgres) : IClassFixture +{ + private static CancellationToken Cancel => TestContext.Current.CancellationToken; + + [Fact] + public void Prints_a_least_privilege_role() + { + Assert.Equal( + """ + -- A least-privilege role for pgcheckup. Review it, then run it as a superuser + -- (rds_superuser on Amazon RDS, cloudsqlsuperuser on Cloud SQL). + CREATE ROLE checkup LOGIN; + -- Set its password with \password checkup, or use your provider's IAM login. + GRANT pg_monitor TO checkup; + GRANT CONNECT ON DATABASE app TO checkup; + -- Every session of this role is read-only, even outside pgcheckup. + ALTER ROLE checkup SET default_transaction_read_only = on; + + -- Only integer-exhaustion needs these. They show sequence counters, never table rows. + -- Repeat them for each schema with sequences. The second covers sequences that app, + -- the role that owns your tables, creates later; name another with --owner. + GRANT SELECT ON ALL SEQUENCES IN SCHEMA public TO checkup; + ALTER DEFAULT PRIVILEGES FOR ROLE app IN SCHEMA public GRANT SELECT ON SEQUENCES TO checkup; + + """.ReplaceLineEndings("\n"), + GrantScript.Build("checkup", "app", "app")); + } + + [Theory] + [InlineData("check\nup")] + [InlineData("check\u001bup")] + public void Refuses_a_name_with_control_characters(string name) + { + // A line break would end the -- comment the name appears in, and run the rest as SQL. + Assert.Throws(() => GrantScript.Build(name, "app", "app")); + Assert.Throws(() => GrantScript.Build("checkup", name, "app")); + Assert.Throws(() => GrantScript.Build("checkup", "app", name)); + } + + [Theory] + [InlineData("user", "\"user\"")] + [InlineData("Checkup", "\"Checkup\"")] + [InlineData("check-up", "\"check-up\"")] + [InlineData("we\"ird", "\"we\"\"ird\"")] + [InlineData("checkup_2", "checkup_2")] + public void Quotes_names_that_need_it(string name, string quoted) + { + Assert.Contains($"CREATE ROLE {quoted} LOGIN;", GrantScript.Build(name, "app", "app")); + Assert.Contains($"GRANT CONNECT ON DATABASE {quoted} TO", GrantScript.Build("checkup", name, "app")); + Assert.Contains($"FOR ROLE {quoted} IN SCHEMA", GrantScript.Build("checkup", "app", name)); + } + + // The printed SQL must actually produce a role that scans cleanly and can't write. + [Fact] + public async Task Creates_a_role_that_scans_every_check_and_cannot_write() + { + // The test server's tables belong to its superuser, postgres. + var statements = FixtureScript.Parse(GrantScript.Build("scanner", "app", "postgres")).Statements.Select(s => s.Sql); + await postgres.Server.ExecuteAsSuperuserAsync(Cancel, [.. statements, "ALTER ROLE scanner PASSWORD 'scanner'"]); + var scanner = postgres.Server.Checkup; + scanner.Username = "scanner"; + scanner.Password = "scanner"; + + await using (var session = await ReadOnlySession.OpenAsync(scanner, Cancel)) + { + var report = await Scanner.ScanAsync(session, "db.example.com", CheckCatalog.All, Cancel); + // Only a newer Postgres version may skip a check; the grant must cover every privilege. + Assert.DoesNotContain(report.Results, r => r.Status == CheckStatus.Errored + || (r.Status == CheckStatus.Skipped && !r.Reason!.StartsWith("needs Postgres", StringComparison.Ordinal))); + } + + // Outside pgcheckup's guards, the role's own default still refuses writes. + await using var plain = new NpgsqlConnection(scanner.ConnectionString); + await plain.OpenAsync(Cancel); + await using var write = new NpgsqlCommand("CREATE TABLE public.scanner_wrote (n int)", plain); + var error = await Assert.ThrowsAsync(() => write.ExecuteNonQueryAsync(Cancel)); + Assert.Equal(PostgresErrorCodes.ReadOnlySqlTransaction, error.SqlState); + } +} diff --git a/tests/Pgcheckup.Tests/Cli/MachineReportTests.cs b/tests/Pgcheckup.Tests/Cli/MachineReportTests.cs new file mode 100644 index 0000000..6f89d6d --- /dev/null +++ b/tests/Pgcheckup.Tests/Cli/MachineReportTests.cs @@ -0,0 +1,161 @@ +using System.Text.Json; +using Pgcheckup.Checks; +using Pgcheckup.Cli; +using Pgcheckup.Engine; + +namespace Pgcheckup.Tests.Cli; + +public class MachineReportTests +{ + private static readonly ServerContext Server = new("app", "db.example.com", 170006, new Provider("rds", "Amazon RDS"), new HashSet()); + + private static readonly Template SlotMessage = new( + [ + new TextPart("Slot "), + new ValuePart("subject", ValueFormat.Default), + new SectionPart([new TextPart(" for "), new ValuePart("inactive_for", ValueFormat.Default)]), + new TextPart(" holds "), + new ValuePart("retained_wal", ValueFormat.Bytes), + ]); + + private static CheckDefinition Check(string id, Template? message = null) => new( + id, $"Title of {id}", "wal", Severity.Warning, 14, [], [], [], "SELECT 1", + message ?? new Template([]), new Template([new ValuePart("slot_literal", ValueFormat.Default)]), ""); + + private static readonly ScanReport Report = new(Server, + [ + new CheckResult(Check("replication-slot-inactive", SlotMessage), CheckStatus.Found, + [ + new Finding("replication-slot-inactive", "debezium", Severity.Warning, + "Slot debezium for 3 days holds 48 GB", + "restart its consumer, or drop the slot:\nSELECT pg_drop_replication_slot('debezium');", + new Dictionary + { + ["subject"] = "debezium", + ["slot_literal"] = "'debezium'", + ["inactive_for"] = new TimeSpan(3, 0, 0, 0, 500), + ["retained_wal"] = 51_539_607_552L, + ["seen_at"] = new DateTime(2026, 9, 25, 14, 3, 59, DateTimeKind.Utc), + }), + ]), + new CheckResult(Check("connection-saturation"), CheckStatus.Passed, []), + new CheckResult(Check("wal-archiving-failing"), CheckStatus.Skipped, [], "managed by Amazon RDS"), + new CheckResult(Check("xid-wraparound"), CheckStatus.Errored, [], "timed out after 5 s"), + ]); + + [Fact] + public void Lists_the_columns_a_template_uses_in_order() + { + Assert.Equal(["subject", "inactive_for", "retained_wal"], SlotMessage.ValueNames); + } + + [Fact] + public void Writes_json_with_schema_1_the_server_and_a_summary() + { + using var json = JsonDocument.Parse(JsonReport.Write(Report, "0.1.0")); + var root = json.RootElement; + + Assert.Equal(1, root.GetProperty("schema").GetInt32()); + Assert.Equal("0.1.0", root.GetProperty("pgcheckup").GetString()); + var server = root.GetProperty("server"); + Assert.Equal("app", server.GetProperty("database").GetString()); + Assert.Equal("db.example.com", server.GetProperty("host").GetString()); + Assert.Equal("17.6", server.GetProperty("version").GetString()); + Assert.Equal("rds", server.GetProperty("provider").GetProperty("id").GetString()); + var summary = root.GetProperty("summary"); + Assert.Equal( + [("passed", 1), ("critical", 0), ("warning", 1), ("info", 0), ("errored", 1), ("skipped", 1)], + summary.EnumerateObject().Select(p => (p.Name, p.Value.GetInt32()))); + } + + [Fact] + public void Writes_each_checks_status_and_reason() + { + using var json = JsonDocument.Parse(JsonReport.Write(Report, "0.1.0")); + + var checks = json.RootElement.GetProperty("checks").EnumerateArray().ToList(); + Assert.Equal( + ["warning", "passed", "skipped", "errored"], + checks.Select(c => c.GetProperty("status").GetString())); + Assert.Equal("managed by Amazon RDS", checks[2].GetProperty("reason").GetString()); + Assert.Equal("timed out after 5 s", checks[3].GetProperty("reason").GetString()); + Assert.False(checks[1].TryGetProperty("reason", out _)); + } + + [Fact] + public void Writes_the_message_columns_of_a_finding_as_raw_values() + { + using var json = JsonDocument.Parse(JsonReport.Write(Report, "0.1.0")); + + var finding = json.RootElement.GetProperty("checks")[0].GetProperty("findings")[0]; + Assert.Equal("debezium", finding.GetProperty("subject").GetString()); + Assert.Equal("warning", finding.GetProperty("severity").GetString()); + Assert.StartsWith("restart its consumer", finding.GetProperty("fix").GetString()); + var values = finding.GetProperty("values"); + Assert.Equal(["subject", "inactive_for", "retained_wal"], values.EnumerateObject().Select(p => p.Name)); + Assert.Equal(259_200.5m, values.GetProperty("inactive_for").GetDecimal()); + Assert.Equal(51_539_607_552L, values.GetProperty("retained_wal").GetInt64()); + } + + [Fact] + public void Writes_markdown_for_a_pull_request_comment() + { + Assert.Equal( + """ + ## pgcheckup · app on db.example.com + + PostgreSQL 17.6 · Amazon RDS · 1 passed · 1 warning · 1 errored · 1 skipped (wal-archiving-failing: managed by Amazon RDS) + + | Severity | Check | Finding | + | --- | --- | --- | + | Warning | `replication-slot-inactive` | Slot debezium for 3 days holds 48 GB | + | Errored | `xid-wraparound` | Timed out after 5 s. | + + ### Fixes + + **`replication-slot-inactive`** · debezium + + ``` + restart its consumer, or drop the slot: + SELECT pg_drop_replication_slot('debezium'); + ``` + + """.ReplaceLineEndings("\n"), + MarkdownReport.Write(Report)); + } + + [Fact] + public void Escapes_pipes_and_line_breaks_in_markdown_cells() + { + var report = new ScanReport(Server, + [ + new CheckResult(Check("a"), CheckStatus.Found, + [new Finding("a", "x", Severity.Critical, "one | two\nthree", "fix", new Dictionary())]), + ]); + + Assert.Contains("| **Critical** | `a` | one \\| two
three |", MarkdownReport.Write(report)); + } + + [Fact] + public void Fences_a_fix_with_more_backticks_than_it_contains() + { + var report = new ScanReport(Server, + [ + new CheckResult(Check("a"), CheckStatus.Found, + [new Finding("a", "x\u001b", Severity.Warning, "A.", "DROP TABLE \"a```b\";", new Dictionary())]), + ]); + + var markdown = MarkdownReport.Write(report); + + Assert.Contains("````\nDROP TABLE \"a```b\";\n````\n", markdown); + Assert.Contains("**`a`** · x\\u001b\n", markdown); + } + + [Fact] + public void Writes_only_the_summary_when_nothing_was_found() + { + var report = new ScanReport(Server with { Provider = null }, [new CheckResult(Check("a"), CheckStatus.Passed, [])]); + + Assert.Equal("## pgcheckup · app on db.example.com\n\nPostgreSQL 17.6 · 1 passed\n", MarkdownReport.Write(report)); + } +} diff --git a/tests/Pgcheckup.Tests/Cli/TerminalReportTests.cs b/tests/Pgcheckup.Tests/Cli/TerminalReportTests.cs index 590ce30..46e23f3 100644 --- a/tests/Pgcheckup.Tests/Cli/TerminalReportTests.cs +++ b/tests/Pgcheckup.Tests/Cli/TerminalReportTests.cs @@ -6,7 +6,7 @@ namespace Pgcheckup.Tests.Cli; public class TerminalReportTests { - private static readonly ServerInfo Server = new("app", "db.example.com", "17.6"); + private static readonly ServerContext Server = new("app", "db.example.com", 170006, null, new HashSet()); private static CheckDefinition Check(string id, Severity severity = Severity.Warning) => new( id, id, "wal", severity, 14, [], [], [], "SELECT 1", new Template([]), new Template([]), ""); @@ -14,26 +14,48 @@ public class TerminalReportTests private static Finding Finding(string checkId, Severity severity, string message, string fix) => new(checkId, "subject", severity, message, fix, new Dictionary()); - private static string Render(ScanReport report, bool color = false) + private static CheckResult Passed(string id) => new(Check(id), CheckStatus.Passed, []); + + private static CheckResult Found(string id, params Finding[] findings) => new(Check(id), CheckStatus.Found, findings); + + private static string Render(ScanReport report, bool color = false, int? width = null) { var output = new StringWriter { NewLine = "\n" }; - TerminalReport.Write(output, report, color); + TerminalReport.Write(output, report, color, width); return output.ToString(); } + [Fact] + public void Wraps_messages_to_the_terminal_width_but_never_the_fix() + { + var report = new ScanReport(Server, + [ + Found("replication-slot-unbounded", Finding("replication-slot-unbounded", Severity.Warning, + "max_slot_wal_keep_size is -1 and this server has replication slots, so one stuck slot can keep WAL until the disk fills.", + "cap it below the free space on the WAL disk:\nALTER SYSTEM SET max_slot_wal_keep_size = '50GB'; SELECT pg_reload_conf();")), + ]); + + Assert.Contains( + """ + WARNING replication-slot-unbounded + max_slot_wal_keep_size is -1 and this server has replication + slots, so one stuck slot can keep WAL until the disk fills. + Fix: cap it below the free space on the WAL disk: + ALTER SYSTEM SET max_slot_wal_keep_size = '50GB'; SELECT pg_reload_conf(); + + """.ReplaceLineEndings("\n"), + Render(report, width: 72)); + } + [Fact] public void Writes_each_finding_with_its_fix_under_a_header_and_above_a_summary() { - var slot = Check("replication-slot-inactive"); var report = new ScanReport(Server, [ - new CheckResult(Check("connection-saturation"), []), - new CheckResult(slot, - [ - Finding(slot.Id, Severity.Warning, - "Slot debezium has been inactive for 3 days and is holding 48 GB of WAL.", - "restart its consumer, or drop the slot:\nSELECT pg_drop_replication_slot('debezium');"), - ]), + Passed("connection-saturation"), + Found("replication-slot-inactive", Finding("replication-slot-inactive", Severity.Warning, + "Slot debezium has been inactive for 3 days and is holding 48 GB of WAL.", + "restart its consumer, or drop the slot:\nSELECT pg_drop_replication_slot('debezium');")), ]); Assert.Equal( @@ -51,34 +73,90 @@ 1 passed · 1 warning Render(report)); } + [Fact] + public void Names_a_managed_provider_in_the_header() + { + var report = new ScanReport(Server with { Provider = new Provider("rds", "Amazon RDS") }, []); + + Assert.StartsWith("pgcheckup · app on db.example.com · PostgreSQL 17.6 · Amazon RDS\n", Render(report)); + } + [Fact] public void Lists_critical_findings_first_and_counts_each_check_once_at_its_worst() { - var slot = Check("replication-slot-inactive"); - var xid = Check("xid-wraparound", Severity.Critical); var report = new ScanReport(Server, [ - new CheckResult(slot, - [ - Finding(slot.Id, Severity.Warning, "Slot a is inactive.", "drop a"), - Finding(slot.Id, Severity.Critical, "Slot b is inactive.", "drop b"), - ]), - new CheckResult(xid, [Finding(xid.Id, Severity.Critical, "Table orders is old.", "vacuum orders")]), - new CheckResult(Check("other-slot"), [Finding("other-slot", Severity.Warning, "Other.", "fix")]), + Found("replication-slot-inactive", + Finding("replication-slot-inactive", Severity.Warning, "Slot a is inactive.", "drop a"), + Finding("replication-slot-inactive", Severity.Critical, "Slot b is inactive.", "drop b")), + Found("xid-wraparound", Finding("xid-wraparound", Severity.Critical, "Table orders is old.", "vacuum orders")), + Found("other-slot", Finding("other-slot", Severity.Warning, "Other.", "fix")), ]); var lines = Render(report).Split('\n'); Assert.Equal( ["CRITICAL replication-slot-inactive", "CRITICAL xid-wraparound", "WARNING other-slot", "WARNING replication-slot-inactive"], - lines.Where(l => l.Length > 0 && !l.StartsWith(' ') && (l.StartsWith("CRITICAL") || l.StartsWith("WARNING")))); + lines.Where(l => l.StartsWith("CRITICAL", StringComparison.Ordinal) || l.StartsWith("WARNING", StringComparison.Ordinal))); Assert.Equal("0 passed · 2 critical · 1 warning", lines[^2]); } + [Fact] + public void Shows_an_errored_check_after_the_findings_with_its_reason() + { + var report = new ScanReport(Server, + [ + new CheckResult(Check("xid-wraparound"), CheckStatus.Errored, [], "timed out after 5 s"), + Found("replication-slot-inactive", Finding("replication-slot-inactive", Severity.Warning, "Slot a is inactive.", "drop a")), + ]); + + Assert.EndsWith( + """ + WARNING replication-slot-inactive + Slot a is inactive. + Fix: drop a + + ERRORED xid-wraparound + Timed out after 5 s. + + 0 passed · 1 warning · 1 errored + + """.ReplaceLineEndings("\n"), + Render(report)); + } + + [Fact] + public void Escapes_control_characters_from_the_server() + { + var report = new ScanReport(Server with { Database = "app\u001b]52;c;x\u0007" }, + [new CheckResult(Check("a"), CheckStatus.Errored, [], "42P01: relation \"t\u001b[2K\" does not exist")]); + + var output = Render(report); + + Assert.DoesNotContain("\u001b", output); + Assert.DoesNotContain("\u0007", output); + Assert.Contains("app\\u001b]52;c;x\\u0007 on", output); + } + + [Fact] + public void Lists_skipped_checks_with_their_reasons_in_the_summary() + { + var report = new ScanReport(Server, + [ + Passed("dangerous-settings"), + new CheckResult(Check("wal-archiving-failing"), CheckStatus.Skipped, [], "managed by Amazon RDS"), + new CheckResult(Check("integer-exhaustion"), CheckStatus.Skipped, [], "can't read sequence counters; see pgcheckup grant"), + ]); + + Assert.EndsWith( + "\n1 passed · 2 skipped (wal-archiving-failing: managed by Amazon RDS, integer-exhaustion: can't read sequence counters; see pgcheckup grant)\n", + Render(report)); + } + [Fact] public void Says_how_many_passed_when_nothing_is_found() { - var report = new ScanReport(Server, [new CheckResult(Check("a"), []), new CheckResult(Check("b"), [])]); + var report = new ScanReport(Server, [Passed("a"), Passed("b")]); Assert.Equal("pgcheckup · app on db.example.com · PostgreSQL 17.6\n\n2 passed\n", Render(report)); } @@ -88,21 +166,26 @@ public void Pluralises_warnings() { var report = new ScanReport(Server, [ - new CheckResult(Check("a"), [Finding("a", Severity.Warning, "A.", "fix")]), - new CheckResult(Check("b"), [Finding("b", Severity.Warning, "B.", "fix")]), + Found("a", Finding("a", Severity.Warning, "A.", "fix")), + Found("b", Finding("b", Severity.Warning, "B.", "fix")), ]); Assert.EndsWith("0 passed · 2 warnings\n", Render(report)); } [Fact] - public void Colours_the_severity_word_only_when_asked() + public void Colours_the_status_word_only_when_asked() { - var report = new ScanReport(Server, [new CheckResult(Check("a"), [Finding("a", Severity.Warning, "A.", "fix")])]); + var report = new ScanReport(Server, + [ + Found("a", Finding("a", Severity.Warning, "A.", "fix")), + new CheckResult(Check("b"), CheckStatus.Errored, [], "timed out after 5 s"), + ]); Assert.DoesNotContain("\u001b", Render(report, color: false)); var coloured = Render(report, color: true); Assert.Contains("\u001b[1;33mWARNING\u001b[0m", coloured); + Assert.Contains("\u001b[1;35mERRORED\u001b[0m", coloured); } [Theory] diff --git a/tests/Pgcheckup.Tests/Engine/ApplicabilityTests.cs b/tests/Pgcheckup.Tests/Engine/ApplicabilityTests.cs new file mode 100644 index 0000000..7953c88 --- /dev/null +++ b/tests/Pgcheckup.Tests/Engine/ApplicabilityTests.cs @@ -0,0 +1,58 @@ +using Pgcheckup.Checks; +using Pgcheckup.Engine; + +namespace Pgcheckup.Tests.Engine; + +public class ApplicabilityTests +{ + private static readonly Provider Rds = new("rds", "Amazon RDS"); + + private static CheckDefinition Check(int minVersion = 14, string[]? privileges = null, string[]? skipOn = null) => new( + "sample-check", "Sample", "wal", Severity.Warning, minVersion, privileges ?? [], skipOn ?? [], [], "SELECT 1", + new Template([]), new Template([]), ""); + + private static ServerContext Server(int versionNumber = 170006, Provider? provider = null, params string[] privileges) => + new("app", "db.example.com", versionNumber, provider, privileges.ToHashSet()); + + [Fact] + public void Runs_a_check_the_server_and_role_can_run() + { + Assert.Null(Applicability.SkipReason(Check(privileges: ["pg_monitor"], skipOn: ["neon"]), Server(provider: Rds, privileges: "pg_monitor"))); + } + + [Fact] + public void Skips_a_check_that_needs_a_newer_postgres() + { + Assert.Equal("needs Postgres 17 or later", Applicability.SkipReason(Check(minVersion: 17), Server(versionNumber: 160004))); + } + + [Fact] + public void Skips_a_check_on_a_provider_that_manages_it() + { + Assert.Equal("managed by Amazon RDS", Applicability.SkipReason(Check(skipOn: ["rds"]), Server(provider: Rds))); + } + + [Fact] + public void Skips_a_check_the_role_lacks_privileges_for() + { + Assert.Equal( + "needs pg_read_all_stats and pg_stat_scan_tables", + Applicability.SkipReason(Check(privileges: ["pg_read_all_stats", "pg_stat_scan_tables"]), Server())); + } + + [Fact] + public void Points_to_grant_when_sequence_counters_are_unreadable() + { + Assert.Equal( + "can't read sequence counters; see pgcheckup grant", + Applicability.SkipReason(Check(privileges: ["select_on_sequences"]), Server())); + } + + [Fact] + public void Gives_the_version_reason_before_the_others() + { + Assert.Equal( + "needs Postgres 17 or later", + Applicability.SkipReason(Check(minVersion: 17, privileges: ["pg_monitor"], skipOn: ["rds"]), Server(versionNumber: 140012, provider: Rds))); + } +} diff --git a/tests/Pgcheckup.Tests/Engine/ScannerTests.cs b/tests/Pgcheckup.Tests/Engine/ScannerTests.cs new file mode 100644 index 0000000..d444f8d --- /dev/null +++ b/tests/Pgcheckup.Tests/Engine/ScannerTests.cs @@ -0,0 +1,80 @@ +using Pgcheckup.Checks; +using Pgcheckup.Engine; +using Pgcheckup.Tests.Postgres; + +namespace Pgcheckup.Tests.Engine; + +public class ScannerTests(PostgresServerFixture postgres) : IClassFixture +{ + private static CancellationToken Cancel => TestContext.Current.CancellationToken; + + private static CheckDefinition Check(string id, string sql, int minVersion = 14) => new( + id, id, "wal", Severity.Warning, minVersion, [], [], [], sql, + new Template([new ValuePart("subject", ValueFormat.Default)]), + new Template([new TextPart("fix it")]), + ""); + + private async Task ScanAsync(params CheckDefinition[] checks) + { + await using var session = await ReadOnlySession.OpenAsync(postgres.Server.Checkup, Cancel); + return await Scanner.ScanAsync(session, "db.example.com", checks, Cancel); + } + + [Fact] + public async Task Reports_a_failing_check_as_errored_and_runs_the_rest() + { + var report = await ScanAsync( + Check("broken-check", "SELECT (1 / 0)::text AS subject"), + Check("working-check", "SELECT 'orders' AS subject")); + + Assert.Collection( + report.Results, + broken => + { + Assert.Equal(CheckStatus.Errored, broken.Status); + Assert.Contains("division by zero", broken.Reason); + }, + working => + { + Assert.Equal(CheckStatus.Found, working.Status); + Assert.Equal("orders", Assert.Single(working.Findings).Subject); + }); + } + + [Fact] + public async Task Reports_a_check_that_runs_too_long_as_timed_out() + { + var result = Assert.Single((await ScanAsync(Check("slow-check", "SELECT 'a' AS subject FROM pg_sleep(6)"))).Results); + + Assert.Equal(CheckStatus.Errored, result.Status); + Assert.Equal("timed out after 5 s", result.Reason); + } + + [Fact] + public async Task Skips_a_check_that_does_not_apply_without_running_it() + { + // The query would fail if it ran. + var result = Assert.Single((await ScanAsync(Check("future-check", "SELECT (1 / 0)::text AS subject", minVersion: 99))).Results); + + Assert.Equal(CheckStatus.Skipped, result.Status); + Assert.Equal("needs Postgres 99 or later", result.Reason); + } + + [Fact] + public async Task Passes_a_check_that_finds_nothing() + { + var result = Assert.Single((await ScanAsync(Check("quiet-check", "SELECT 'a' AS subject WHERE false"))).Results); + + Assert.Equal(CheckStatus.Passed, result.Status); + Assert.Null(result.Reason); + } + + [Fact] + public async Task Carries_the_server_context() + { + var report = await ScanAsync(); + + Assert.Equal("app", report.Server.Database); + Assert.Equal(int.Parse(PostgresServer.Version), report.Server.Major); + } +} diff --git a/tests/Pgcheckup.Tests/Engine/ServerContextTests.cs b/tests/Pgcheckup.Tests/Engine/ServerContextTests.cs new file mode 100644 index 0000000..72cb371 --- /dev/null +++ b/tests/Pgcheckup.Tests/Engine/ServerContextTests.cs @@ -0,0 +1,95 @@ +using Pgcheckup.Engine; +using Pgcheckup.Tests.Postgres; + +namespace Pgcheckup.Tests.Engine; + +public class ServerContextTests(PostgresServerFixture postgres) : IClassFixture +{ + private static CancellationToken Cancel => TestContext.Current.CancellationToken; + + private async Task ReadAsync(Npgsql.NpgsqlConnectionStringBuilder? role = null) + { + await using var session = await ReadOnlySession.OpenAsync(role ?? postgres.Server.Checkup, Cancel); + return await ServerContext.ReadAsync(session, "db.example.com", Cancel); + } + + // Roles are cluster-wide, so each case removes what it created. + public static TheoryData Providers() => new() + { + { [], [], null }, + { ["CREATE ROLE rds_superuser"], ["DROP ROLE rds_superuser"], "rds" }, + { + ["CREATE ROLE rds_superuser", "CREATE FUNCTION public.aurora_version() RETURNS text LANGUAGE sql AS $$ SELECT '16.4.0' $$"], + ["DROP FUNCTION public.aurora_version()", "DROP ROLE rds_superuser"], + "aurora" + }, + { ["CREATE ROLE cloudsqlsuperuser"], ["DROP ROLE cloudsqlsuperuser"], "cloudsql" }, + { ["CREATE ROLE azure_pg_admin"], ["DROP ROLE azure_pg_admin"], "azure" }, + { ["CREATE ROLE supabase_admin"], ["DROP ROLE supabase_admin"], "supabase" }, + { ["ALTER DATABASE app SET neon.tenant_id = 'placeholder'"], ["ALTER DATABASE app RESET neon.tenant_id"], "neon" }, + }; + + [Theory] + [MemberData(nameof(Providers))] + public async Task Detects_the_managed_provider_from_its_roles_and_settings(string[] setup, string[] teardown, string? expected) + { + await postgres.Server.ExecuteAsSuperuserAsync(Cancel, setup); + try + { + Assert.Equal(expected, (await ReadAsync()).Provider?.Id); + } + finally + { + await postgres.Server.ExecuteAsSuperuserAsync(Cancel, teardown); + } + } + + [Fact] + public async Task Reads_the_database_and_version() + { + var context = await ReadAsync(); + + Assert.Equal("app", context.Database); + Assert.Equal("db.example.com", context.Host); + Assert.Equal(int.Parse(PostgresServer.Version), context.Major); + Assert.StartsWith($"{PostgresServer.Version}.", context.Version); + } + + [Fact] + public async Task Knows_the_privileges_that_pg_monitor_brings() + { + var context = await ReadAsync(); + + Assert.Superset( + new HashSet { "pg_monitor", "pg_read_all_settings", "pg_read_all_stats", "pg_stat_scan_tables" }, + new HashSet(context.Privileges)); + } + + [Fact] + public async Task Knows_a_plain_role_has_none_of_them() + { + await postgres.Server.ExecuteAsSuperuserAsync(Cancel, "CREATE ROLE plain LOGIN PASSWORD 'plain'"); + var plain = postgres.Server.Checkup; + plain.Username = "plain"; + plain.Password = "plain"; + + Assert.DoesNotContain((await ReadAsync(plain)).Privileges, p => p.StartsWith("pg_", StringComparison.Ordinal)); + } + + [Fact] + public async Task Can_read_sequence_counters_only_once_granted() + { + await postgres.Server.ExecuteAsSuperuserAsync(Cancel, "CREATE SEQUENCE public.counter"); + try + { + Assert.DoesNotContain("select_on_sequences", (await ReadAsync()).Privileges); + + await postgres.Server.ExecuteAsSuperuserAsync(Cancel, "GRANT SELECT ON SEQUENCE public.counter TO checkup"); + Assert.Contains("select_on_sequences", (await ReadAsync()).Privileges); + } + finally + { + await postgres.Server.ExecuteAsSuperuserAsync(Cancel, "DROP SEQUENCE public.counter"); + } + } +} diff --git a/tests/Pgcheckup.Tests/Postgres/FixtureScript.cs b/tests/Pgcheckup.Tests/Postgres/FixtureScript.cs index 46491f8..f829516 100644 --- a/tests/Pgcheckup.Tests/Postgres/FixtureScript.cs +++ b/tests/Pgcheckup.Tests/Postgres/FixtureScript.cs @@ -5,26 +5,42 @@ namespace Pgcheckup.Tests.Postgres; -// A check's fixture: setup statements, plus `-- threshold name = value` lines that lower a -// threshold when the real condition can't be reproduced at full scale. +/// One statement of a fixture. +/// The statement, with any comments before it. +/// Whether an -- expect error line comes before it, so it must fail. +internal sealed record FixtureStatement(string Sql, bool MayFail); + +/// +/// A check's fixture: setup statements, plus directives. -- threshold name = value lowers a +/// threshold when the real condition can't be reproduced at full scale. -- server name = value +/// starts Postgres with a setting that needs a restart. -- expect error means the next +/// statement must fail, as a failed CREATE INDEX CONCURRENTLY does. +/// internal sealed partial class FixtureScript { - private FixtureScript(IReadOnlyList statements, IReadOnlyDictionary thresholds) + private FixtureScript( + IReadOnlyList statements, + IReadOnlyDictionary thresholds, + IReadOnlyDictionary serverSettings) { Statements = statements; Thresholds = thresholds; + ServerSettings = serverSettings; } - public IReadOnlyList Statements { get; } + public IReadOnlyList Statements { get; } public IReadOnlyDictionary Thresholds { get; } + public IReadOnlyDictionary ServerSettings { get; } + public static FixtureScript Load(string checkId, string fixture) => Parse(File.ReadAllText(Path.Combine(AppContext.BaseDirectory, "checks", checkId, "fixtures", fixture + ".sql"))); public static FixtureScript Parse(string text) { var thresholds = ThresholdLine().Matches(text).ToDictionary(m => m.Groups[1].Value, m => m.Groups[2].Value.Trim()); + var serverSettings = ServerLine().Matches(text).ToDictionary(m => m.Groups[1].Value, m => m.Groups[2].Value.Trim()); var errors = new List(); var tokens = SqlTokenizer.Tokenize(text, errors); @@ -33,19 +49,20 @@ public static FixtureScript Parse(string text) throw new InvalidOperationException($"The fixture doesn't parse: {string.Join("; ", errors)}"); } - var statements = new List(); + var statements = new List(); var start = 0; foreach (var end in tokens.Where(t => t.Kind == TokenKind.Semicolon).Select(t => t.Start).Append(text.Length)) { if (tokens.Any(t => t.Start >= start && t.Start < end && t.Kind != TokenKind.Semicolon)) { - statements.Add(text[start..end].Trim()); + var sql = text[start..end].Trim(); + statements.Add(new FixtureStatement(sql, ExpectErrorLine().IsMatch(sql))); } start = end + 1; } - return new FixtureScript(statements, thresholds); + return new FixtureScript(statements, thresholds, serverSettings); } public CheckDefinition Apply(CheckDefinition check) @@ -77,4 +94,10 @@ public CheckDefinition Apply(CheckDefinition check) [GeneratedRegex(@"^--\s*threshold\s+([a-z][a-z0-9_]*)\s*=\s*(.+)$", RegexOptions.Multiline)] private static partial Regex ThresholdLine(); + + [GeneratedRegex(@"^--\s*server\s+([a-z][a-z0-9_.]*)\s*=\s*(.+)$", RegexOptions.Multiline)] + private static partial Regex ServerLine(); + + [GeneratedRegex(@"^--\s*expect error\s*$", RegexOptions.Multiline)] + private static partial Regex ExpectErrorLine(); } diff --git a/tests/Pgcheckup.Tests/Postgres/FixtureScriptTests.cs b/tests/Pgcheckup.Tests/Postgres/FixtureScriptTests.cs new file mode 100644 index 0000000..4868c13 --- /dev/null +++ b/tests/Pgcheckup.Tests/Postgres/FixtureScriptTests.cs @@ -0,0 +1,48 @@ +namespace Pgcheckup.Tests.Postgres; + +public class FixtureScriptTests +{ + [Fact] + public void Splits_statements_on_semicolons_outside_literals() + { + var script = FixtureScript.Parse(""" + CREATE TABLE t (n int); + DO $$ BEGIN PERFORM 1; END $$; + -- a comment; with a semicolon + SELECT 'a;b'; + """); + + Assert.Equal( + ["CREATE TABLE t (n int)", "DO $$ BEGIN PERFORM 1; END $$", "-- a comment; with a semicolon\nSELECT 'a;b'"], + script.Statements.Select(s => s.Sql)); + } + + [Fact] + public void Reads_threshold_and_server_directives() + { + var script = FixtureScript.Parse(""" + -- threshold min_age = 0s + -- server max_prepared_transactions = 5 + -- server archive_mode = on + SELECT 1; + """); + + Assert.Equal(new Dictionary { ["min_age"] = "0s" }, script.Thresholds); + Assert.Equal( + new Dictionary { ["max_prepared_transactions"] = "5", ["archive_mode"] = "on" }, + script.ServerSettings); + } + + [Fact] + public void Lets_only_the_statement_after_expect_error_fail() + { + var script = FixtureScript.Parse(""" + CREATE TABLE t (n int); + -- expect error + CREATE UNIQUE INDEX CONCURRENTLY t_n ON t (n); + SELECT 1; + """); + + Assert.Equal([false, true, false], script.Statements.Select(s => s.MayFail)); + } +} diff --git a/tests/Pgcheckup.Tests/Postgres/PostgresServer.cs b/tests/Pgcheckup.Tests/Postgres/PostgresServer.cs index 8cc6521..9628e1e 100644 --- a/tests/Pgcheckup.Tests/Postgres/PostgresServer.cs +++ b/tests/Pgcheckup.Tests/Postgres/PostgresServer.cs @@ -1,3 +1,4 @@ +using DotNet.Testcontainers.Configurations; using Npgsql; using Testcontainers.PostgreSql; @@ -23,9 +24,19 @@ public sealed class PostgresServer : IAsyncDisposable Password = "checkup", }; - public static async Task StartAsync(CancellationToken cancellationToken) + public static Task StartAsync(CancellationToken cancellationToken) => + StartAsync(new Dictionary(), cancellationToken); + + // Replaces Testcontainers' default command, which turns fsync, full_page_writes and + // synchronous_commit off, so fixtures run on stock Postgres plus what they ask for. + public static async Task StartAsync(IReadOnlyDictionary settings, CancellationToken cancellationToken) { - var container = new PostgreSqlBuilder($"postgres:{Version}-alpine").WithDatabase("app").Build(); + var command = settings.SelectMany(s => new[] { "-c", $"{s.Key}={s.Value}" }).ToArray(); + // Debian, not Alpine: most servers run glibc, whose collation versions a check compares. + var container = new PostgreSqlBuilder($"postgres:{Version}") + .WithDatabase("app") + .WithCommand(new OverwriteEnumerable(command)) + .Build(); await container.StartAsync(cancellationToken); var server = new PostgresServer(container); await server.ExecuteAsSuperuserAsync(cancellationToken, "CREATE ROLE checkup LOGIN PASSWORD 'checkup' IN ROLE pg_monitor"); diff --git a/tests/Pgcheckup.Tests/Postgres/PostgresServerTests.cs b/tests/Pgcheckup.Tests/Postgres/PostgresServerTests.cs new file mode 100644 index 0000000..e262547 --- /dev/null +++ b/tests/Pgcheckup.Tests/Postgres/PostgresServerTests.cs @@ -0,0 +1,35 @@ +using Npgsql; + +namespace Pgcheckup.Tests.Postgres; + +// Fixtures must run on stock Postgres, or a check that looks for risky settings would fire on +// every fixture. Testcontainers turns fsync, full_page_writes and synchronous_commit off by default. +public class PostgresServerTests +{ + private static CancellationToken Cancel => TestContext.Current.CancellationToken; + + private static async Task ShowAsync(PostgresServer server, string setting) + { + await using var connection = await server.OpenSuperuserAsync(Cancel); + await using var command = new NpgsqlCommand($"SHOW {setting}", connection); + return (string)(await command.ExecuteScalarAsync(Cancel))!; + } + + [Fact] + public async Task Starts_with_stock_settings() + { + await using var server = await PostgresServer.StartAsync(Cancel); + + Assert.Equal("on", await ShowAsync(server, "fsync")); + Assert.Equal("on", await ShowAsync(server, "full_page_writes")); + Assert.Equal("on", await ShowAsync(server, "synchronous_commit")); + } + + [Fact] + public async Task Starts_with_the_settings_a_fixture_asks_for() + { + await using var server = await PostgresServer.StartAsync(new Dictionary { ["max_prepared_transactions"] = "5" }, Cancel); + + Assert.Equal("5", await ShowAsync(server, "max_prepared_transactions")); + } +}