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.

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:

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,
+    <source_last_value> + 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

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) |

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
 

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)
 

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.