From a507eee6c0d5e143fa5a32b5eaab7e72042532c4 Mon Sep 17 00:00:00 2001 From: sav-maya Date: Thu, 20 Aug 2026 11:57:37 -0700 Subject: [PATCH] Fix PostgreSQL reference review comments Correct transaction retry semantics, recovery behavior, role versions, configuration scope, foreign-key defaults, and logical replication diagnostics based on reviewer feedback. --- .../references/backup-restore.md | 9 ++- .../references/logical-replication.md | 65 +++++++++++++--- .../references/query-optimization.md | 4 +- .../references/schema-design.md | 6 +- .../references/security-roles.md | 10 +-- .../references/transaction-isolation.md | 75 ++++++++++++------- 6 files changed, 117 insertions(+), 52 deletions(-) diff --git a/skills/postgres-best-practices/references/backup-restore.md b/skills/postgres-best-practices/references/backup-restore.md index f46bf25..2ee52a3 100644 --- a/skills/postgres-best-practices/references/backup-restore.md +++ b/skills/postgres-best-practices/references/backup-restore.md @@ -180,7 +180,7 @@ pg_basebackup -h primary_host -U repl_user -D /backup/base -Fp -Xs -P ### Prerequisites -The user running `pg_basebackup` needs `REPLICATION` privilege or membership in `pg_write_server_files`: +The connection used by `pg_basebackup` must authenticate as a superuser or a role with `REPLICATION` privilege. `max_wal_senders` must also have enough capacity for the backup connection: ```sql -- On the primary @@ -193,6 +193,8 @@ And `pg_hba.conf` must allow replication connections: host replication backup_user backup_server_ip/32 scram-sha-256 ``` +[PG15+] `--target=server:/path` writes the backup on the database server. A non-superuser using this target needs both `REPLICATION` privilege for the backup connection and membership in `pg_write_server_files` for the server-side write. `--target` is unavailable in PG14 and cannot be combined with `-Xstream`; use `-Xfetch` or `-Xnone`. + ## Point-in-Time Recovery (PITR) PITR lets you restore a database to any specific point in time — invaluable for recovering from accidental data deletion or corruption. @@ -259,13 +261,16 @@ recovery_target_xid = '12345678' -- Recover to a named restore point recovery_target_name = 'before_migration' --- Recover to end of available WAL (latest possible state) +-- Stop as soon as a consistent state is reached +-- (for an online backup, normally the point where the backup ended) recovery_target = 'immediate' -- Recover to a specific WAL position recovery_target_lsn = '0/1A2B3C4D' ``` +To recover through the end of all available WAL, omit every `recovery_target*` setting. `recovery_target = 'immediate'` is an explicit early stopping target, so later available WAL can remain unapplied. PostgreSQL may still replay the WAL required to make an online backup consistent. + ### Creating Named Restore Points Before risky operations, create a named restore point: diff --git a/skills/postgres-best-practices/references/logical-replication.md b/skills/postgres-best-practices/references/logical-replication.md index e1a2634..f48612b 100644 --- a/skills/postgres-best-practices/references/logical-replication.md +++ b/skills/postgres-best-practices/references/logical-replication.md @@ -334,12 +334,22 @@ WHERE subname IS NOT NULL; -- Per-table sync state (initial copy progress) SELECT - srsubid::regclass AS subscription, - srrelid::regclass AS table_name, - srsubstate AS state, - -- 'i' = init, 'd' = data copy, 'f' = finished table copy, 's' = synced, 'r' = ready - srsublsn AS lsn -FROM pg_subscription_rel; + s.subname AS subscription, + sr.srrelid::regclass AS table_name, + sr.srsubstate AS state, + CASE sr.srsubstate + WHEN 'i' THEN 'initialize' + WHEN 'd' THEN 'data is being copied' + WHEN 'f' THEN 'finished table copy' + WHEN 's' THEN 'synchronized' + WHEN 'r' THEN 'ready (normal replication)' + ELSE 'unknown' + END AS state_meaning, + sr.srsublsn AS lsn +FROM pg_catalog.pg_subscription_rel AS sr +JOIN pg_catalog.pg_subscription AS s + ON s.oid = sr.srsubid +ORDER BY s.subname, sr.srrelid::regclass::text; ``` **State codes for `pg_subscription_rel.srsubstate`:** @@ -352,6 +362,8 @@ FROM pg_subscription_rel; | `s` | Synced with publisher | | `r` | Ready (streaming) | +These codes describe each table's initialization state. `s` means the table synchronized during initialization; it does not prove that current replication lag is zero. + ### Replication Lag Monitoring Query Run on the subscriber to check how far behind it is: @@ -435,12 +447,17 @@ Logical replication does **NOT** replicate sequences. Before cutover, sync them: ```sql -- On SOURCE: get current sequence values -SELECT sequencename, last_value +SELECT schemaname, sequencename, last_value FROM pg_sequences WHERE schemaname = 'public'; --- On TARGET: set sequences to match (add buffer for safety) -SELECT setval('orders_id_seq', (SELECT last_value FROM pg_sequences WHERE sequencename = 'orders_id_seq') + 1000); +-- On TARGET: substitute the value obtained from the source. +-- Add a buffer only if writes can still reach the source during cutover. +SELECT pg_catalog.setval( + 'public.orders_id_seq'::regclass, + + 1000, + true +); ``` ### Row Count Verification @@ -459,16 +476,42 @@ ORDER BY relname; For exact counts (slower): ```sql +CREATE TEMP TABLE exact_row_counts ( + schemaname name NOT NULL, + tablename name NOT NULL, + exact_count bigint NOT NULL +); + DO $$ DECLARE r record; BEGIN - FOR r IN SELECT tablename FROM pg_tables WHERE schemaname = 'public' ORDER BY tablename + FOR r IN + SELECT schemaname, tablename + FROM pg_catalog.pg_tables + WHERE schemaname = 'public' + ORDER BY tablename LOOP - EXECUTE format('SELECT %L AS table_name, count(*) AS exact_count FROM %I', r.tablename, r.tablename); + EXECUTE format( + 'INSERT INTO pg_temp.exact_row_counts + SELECT %L::name, %L::name, count(*) + FROM %I.%I', + r.schemaname, r.tablename, + r.schemaname, r.tablename + ); END LOOP; END $$; + +SELECT + format('%I.%I', schemaname, tablename) AS table_name, + exact_count +FROM pg_temp.exact_row_counts +ORDER BY schemaname, tablename; + +DROP TABLE pg_temp.exact_row_counts; ``` +Run the exact-count query on both source and target only after stopping writes and allowing replication lag to drain. It performs a full scan of every selected table. + ## Troubleshooting ### Replication Slot Growing / WAL Accumulation diff --git a/skills/postgres-best-practices/references/query-optimization.md b/skills/postgres-best-practices/references/query-optimization.md index a0a7824..d4ef26e 100644 --- a/skills/postgres-best-practices/references/query-optimization.md +++ b/skills/postgres-best-practices/references/query-optimization.md @@ -249,11 +249,11 @@ WHERE tablename = 'orders' AND attname = 'status'; ## Configuration Tuning Knobs -These are per-session overridable. Test with `SET` before changing `postgresql.conf`. +Except for `shared_buffers`, these parameters can be overridden in the current session with `SET` for workload-specific testing. `shared_buffers` is allocated at server start; persisting a change in `postgresql.conf` or with `ALTER SYSTEM` requires a restart before it takes effect. | Parameter | Default | When to increase | |-----------|---------|-----------------| -| `shared_buffers` | 128MB | Set to ~25% of total RAM. See performance-diagnostics for cache hit rate queries | +| `shared_buffers` | 128MB | Server-wide, restart-required setting; start around 25% of total RAM and validate for the workload. See performance-diagnostics for cache hit rate queries | | `work_mem` | 4MB | Sort/hash spilling to disk | | `maintenance_work_mem` | 64MB | Slow VACUUM, CREATE INDEX, ALTER TABLE | | `effective_cache_size` | 4GB | Set to ~75% of total RAM (hint to planner, no allocation) | diff --git a/skills/postgres-best-practices/references/schema-design.md b/skills/postgres-best-practices/references/schema-design.md index bc1de85..6490e0f 100644 --- a/skills/postgres-best-practices/references/schema-design.md +++ b/skills/postgres-best-practices/references/schema-design.md @@ -112,11 +112,11 @@ CREATE TABLE orders ( | Action | Use when | |--------|----------| -| `RESTRICT` (default) | Prevent accidental deletion of referenced rows | +| `NO ACTION` (default) | Reject the update or deletion if a violation remains when the constraint is checked; the check can be deferred when the foreign key is deferrable and currently deferred | +| `RESTRICT` | Reject the referenced operation immediately; the check cannot be deferred | | `CASCADE` | Child rows are meaningless without parent (e.g., order_items when order is deleted) | | `SET NULL` | Relationship is optional, preserve child row | -| `SET DEFAULT` | Rare; reassign to a default parent | -| `NO ACTION` | Like RESTRICT but deferrable | +| `SET DEFAULT` | Rare; reassign to a valid default parent | ### Deferrable Foreign Keys diff --git a/skills/postgres-best-practices/references/security-roles.md b/skills/postgres-best-practices/references/security-roles.md index 4abfed8..8d3388d 100644 --- a/skills/postgres-best-practices/references/security-roles.md +++ b/skills/postgres-best-practices/references/security-roles.md @@ -77,9 +77,9 @@ JOIN pg_roles m ON m.oid = am.member ORDER BY r.rolname, m.rolname; ``` -### Predefined Roles (PG14+) +### Predefined Roles -PostgreSQL provides built-in roles for common needs: +PostgreSQL provides system-defined roles for common privileged capabilities. PostgreSQL 13 and earlier documentation called these "default roles"; PostgreSQL 14 renamed the category to "predefined roles." Individual roles were introduced in different releases. | Role | Grants | |------|--------| @@ -90,7 +90,7 @@ PostgreSQL provides built-in roles for common needs: | `pg_monitor` | Read monitoring views (`pg_stat_*`, `pg_locks`, etc.) | | `pg_signal_backend` | Send signals to other backends (cancel/terminate) | | `pg_checkpoint` (PG15+) | Run CHECKPOINT | -| `pg_maintain` (PG15+) | Run VACUUM, ANALYZE, REINDEX, CLUSTER, REFRESH MATERIALIZED VIEW | +| `pg_maintain` (PG17+) | Run VACUUM, ANALYZE, REINDEX, CLUSTER, REFRESH MATERIALIZED VIEW, and LOCK TABLE on all relations | ```sql -- Give a monitoring role read access to all stats @@ -217,13 +217,13 @@ ALTER ROLE app_user SET search_path = app, public; -- For functions: use SECURITY DEFINER carefully CREATE FUNCTION get_balance(account_id int) RETURNS numeric SECURITY DEFINER - SET search_path = app, pg_temp -- pin the search path + SET search_path = pg_catalog, pg_temp AS $$ SELECT balance FROM app.accounts WHERE id = account_id; $$ LANGUAGE sql; ``` -**Always set `search_path` in `SECURITY DEFINER` functions** to prevent search path manipulation attacks. +**Always give `SECURITY DEFINER` functions a fixed `search_path` containing only trusted schemas.** Explicitly list `pg_temp` last: if it is omitted, PostgreSQL searches the session's temporary schema first for relations and data types, which can allow temporary objects to shadow intended objects. When application objects are fully qualified, prefer `pg_catalog, pg_temp`; otherwise list only trusted application schemas followed by `pg_temp`. ## Row-Level Security (RLS) diff --git a/skills/postgres-best-practices/references/transaction-isolation.md b/skills/postgres-best-practices/references/transaction-isolation.md index 96145c2..0a7ca89 100644 --- a/skills/postgres-best-practices/references/transaction-isolation.md +++ b/skills/postgres-best-practices/references/transaction-isolation.md @@ -275,53 +275,70 @@ DDL statements (ALTER TABLE, CREATE INDEX) always acquire strong locks regardles ```python # Python example (psycopg) -import psycopg -from psycopg.errors import SerializationFailure +import random +import time -MAX_RETRIES = 3 +import psycopg +from psycopg.errors import DeadlockDetected, SerializationFailure + +MAX_RETRIES = 5 +BASE_DELAY_SECONDS = 0.01 +MAX_DELAY_SECONDS = 0.5 for attempt in range(MAX_RETRIES): try: + # This must be a top-level transaction, not a savepoint inside one. with conn.transaction(): + conn.execute("SET TRANSACTION ISOLATION LEVEL SERIALIZABLE") conn.execute("UPDATE accounts SET balance = balance - 100 WHERE id = 1") conn.execute("UPDATE accounts SET balance = balance + 100 WHERE id = 2") break # success - except SerializationFailure: + except (SerializationFailure, DeadlockDetected): if attempt == MAX_RETRIES - 1: raise # give up after max retries - continue # retry the entire transaction + + # Bounded exponential backoff with jitter reduces repeated collisions. + delay = min(MAX_DELAY_SECONDS, BASE_DELAY_SECONDS * (2 ** attempt)) + time.sleep(random.uniform(delay / 2, delay)) ``` ### Key Retry Rules 1. **Retry the entire transaction** — not just the failed statement -2. **Use a bounded retry count** — avoid infinite loops (3-5 retries is typical) -3. **Don't add sleep/backoff** for serialization retries — they're usually instant to resolve -4. **Log retries** for monitoring — frequent retries indicate high contention -5. **SQLSTATE 40001** is the error code to catch (serialization failure) -6. **SQLSTATE 40P01** is deadlock — also safe to retry with the same pattern +2. **Re-run all reads and decision-making** — transaction inputs may no longer be valid after a conflict +3. **Use a bounded retry count** — avoid infinite retry loops +4. **Use bounded exponential backoff with jitter** — concurrent retriers should not repeatedly collide; tune the delays for the workload +5. **Log retries and exhaustion** — frequent retries indicate contention or transaction-design problems +6. **SQLSTATE 40001** is the error code to catch (`serialization_failure`) +7. **SQLSTATE 40P01** is deadlock (`deadlock_detected`) — it can be retried, but also fix inconsistent lock ordering where possible +8. **Do not publish external side effects before commit** — messages, emails, and API calls can otherwise be duplicated by a retry -### PL/pgSQL Retry +PostgreSQL requires retrying the complete transaction and warns that multiple attempts may be needed. See [Serialization Failure Handling](https://www.postgresql.org/docs/current/mvcc-serialization-failure-handling.html). + +### PL/pgSQL Procedure for the Transaction Body + +A PL/pgSQL function cannot retry a complete transaction. An `EXCEPTION` block rolls back only its subtransaction; retrying the block remains inside the same top-level transaction and uses the same transaction snapshot at REPEATABLE READ or SERIALIZABLE. + +A procedure can encapsulate the database work, but the application should still own the complete transaction and retry loop: ```sql -CREATE OR REPLACE FUNCTION transfer_funds(from_id int, to_id int, amount numeric) -RETURNS void AS $$ -DECLARE - retries int := 0; +CREATE OR REPLACE PROCEDURE transfer_funds( + p_from_id int, + p_to_id int, + p_amount numeric +) +LANGUAGE plpgsql +AS $$ BEGIN - LOOP - BEGIN - UPDATE accounts SET balance = balance - amount WHERE id = from_id; - UPDATE accounts SET balance = balance + amount WHERE id = to_id; - RETURN; - EXCEPTION - WHEN serialization_failure OR deadlock_detected THEN - retries := retries + 1; - IF retries >= 3 THEN - RAISE; - END IF; - END; - END LOOP; + UPDATE accounts + SET balance = balance - p_amount + WHERE id = p_from_id; + + UPDATE accounts + SET balance = balance + p_amount + WHERE id = p_to_id; END; -$$ LANGUAGE plpgsql; +$$; ``` + +Invoke `CALL transfer_funds(...)` inside the application-managed transaction and retry that entire transaction from the application. A serialization failure can occur at commit, so a procedure cannot reliably catch every failure and restart itself.