Skip to content

Feature/Adding Info and System Level Checks#29

Merged
randoneering merged 27 commits into
mainfrom
feature/system_info_checks
Apr 13, 2026
Merged

Feature/Adding Info and System Level Checks#29
randoneering merged 27 commits into
mainfrom
feature/system_info_checks

Conversation

@randoneering
Copy link
Copy Markdown
Owner

@randoneering randoneering commented Apr 12, 2026

Pull Request Summary

Adds 10 new health checks covering PostgreSQL memory settings, server identity, and operational stats. All checks use built-in system catalogs and functions — no extensions required.

  • shared_buffers Setting: INFO — current_setting()
  • shared_buffers At Default: HIGH — fires when value is the 128MB default
  • work_mem Setting: INFO — current_setting()
  • work_mem At Default: MEDIUM — fires when value is the 4MB default
  • effective_cache_size Setting: INFO — current_setting()
  • maintenance_work_mem Setting: INFO — current_setting()
  • Transaction ID Wraparound Risk: INFO — pg_database — one row per database showing XID age

Type of Change

  • New health check
  • Bug fix
  • Performance improvement
  • Documentation update
  • Refactoring/code cleanup
  • Breaking change

Related Issues

  • Fixes #
  • Related to #
  • Closes #

Testing

PostgreSQL Version Compatibility

Has this code been tested against the following PostgreSQL versions?

  • PostgreSQL 15
  • PostgreSQL 16
  • PostgreSQL 17
  • PostgreSQL 18

Testing notes:

All checks use current_setting(), pg_database, pg_stat_bgwriter, pg_stat_activity, and pg_is_in_recovery() — all available since PostgreSQL 9.6+.

Managed Database Platforms

Has this code been deployed and tested on the following platforms?

  • Amazon RDS for PostgreSQL
  • Google Cloud SQL for PostgreSQL
  • Azure Database for PostgreSQL
  • Self-managed PostgreSQL

Platform-specific notes:

No privilege-restricted functions used. All checks should work on managed platforms without modification.


Additional Notes

The Transaction ID Wraparound Risk check returns one row per connectable database (datallowconn = true), ordered by XID age descending — highest risk first.


Summary by CodeRabbit

  • New Features

    • Added comprehensive system health checks including PostgreSQL server configuration parameters (shared_buffers, work_mem, effective_cache_size, maintenance_work_mem) with alerts for non-optimal defaults.
    • Added monitoring for transaction ID wraparound risk, checkpoint statistics, server role detection (primary vs standby), and connection utilization.
  • Tests

    • Added test assertions to verify new health checks are present and functioning correctly.

Copy link
Copy Markdown

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 12, 2026

📝 Walkthrough

Walkthrough

This pull request adds comprehensive system health and configuration monitoring checks to PostgreSQL diagnostics tooling. Changes include new health check logic in the main function, extended view definitions with configuration parameter monitoring, transaction ID wraparound risk tracking, and corresponding test coverage expansion. Git ignore rules were also updated for AI-related file artifacts.

Changes

Cohort / File(s) Summary
Configuration & Metadata
.gitignore
Updated ignore rules for AI-related files: normalized comment casing from "AI Files" to "AI files", restored .claude entry, added patterns for AGENTS.md and docs/superpowers/plans/.
System Health Check Logic
pgFirstAid.sql, view_pgFirstAid.sql, view_pgFirstAid_managed.sql
Added comprehensive system health monitoring checks covering: PostgreSQL configuration parameters (shared_buffers, work_mem, effective_cache_size, maintenance_work_mem) with default-value alerts, per-database transaction ID wraparound risk, checkpoint statistics, server role detection, and connection utilization metrics. Includes conditional HIGH/MEDIUM severity logic for default configuration thresholds.
Test Coverage Expansion
testing/pgTAP/03_high_tests.sql, testing/pgTAP/04_medium_tests.sql, testing/pgTAP/06_info_tests.sql
Increased test plan counts and added assertion suites verifying presence of new system health checks (shared_buffers At Default, work_mem At Default, configuration settings, wraparound risk, checkpoint stats, server role, and connection utilization) in both function output and view results.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~70 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Feature/Adding Info and System Level Checks' is directly related to the changeset, clearly conveying that the PR adds new informational and system-level health checks.
Description check ✅ Passed The PR description includes a clear summary of the 10 new checks added, specifies the type of change (new health check), provides testing notes for PostgreSQL version compatibility and platform considerations, and adds relevant implementation details.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/system_info_checks

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Add 10 system health and info checks for memory and operational monitoring

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add 8 new INFO-level health checks for system configuration
• Add 2 severity-level checks (HIGH, MEDIUM) for default settings
• Checks cover memory settings, server role, XID wraparound, checkpoints
• Update test suite with 16 new test cases for all checks
Diagram
flowchart LR
  A["Health Check System"] --> B["Memory Settings"]
  A --> C["Server State"]
  A --> D["Operational Stats"]
  B --> B1["shared_buffers INFO/HIGH"]
  B --> B2["work_mem INFO/MEDIUM"]
  B --> B3["effective_cache_size INFO"]
  B --> B4["maintenance_work_mem INFO"]
  C --> C1["Server Role"]
  C --> C2["XID Wraparound Risk"]
  D --> D1["Checkpoint Stats"]
  D --> D2["Connection Utilization"]
Loading

Grey Divider

File Changes

1. pgFirstAid.sql ✨ Enhancement +151/-0

Add 10 health checks to main function

• Add 8 new INSERT statements for INFO-level checks using current_setting() and system catalogs
• Add 2 new INSERT statements for HIGH/MEDIUM severity checks on default settings
• Checks query pg_database, pg_stat_bgwriter, and pg_stat_activity for operational data
• All checks include severity, category, descriptions, and documentation links

pgFirstAid.sql


2. view_pgFirstAid.sql ✨ Enhancement +141/-1

Add 10 health checks to main view

• Add identical 10 health checks as UNION ALL queries to the view definition
• Maintains consistency with pgFirstAid.sql function implementation
• Checks include memory settings, server role, XID age, checkpoint stats, and connection utilization

view_pgFirstAid.sql


3. view_pgFirstAid_managed.sql ✨ Enhancement +141/-1

Add 10 health checks to managed view

• Add identical 10 health checks as UNION ALL queries for managed database platforms
• Ensures feature parity with self-managed PostgreSQL implementations
• All checks use platform-agnostic functions available on managed services

view_pgFirstAid_managed.sql


View more (3)
4. testing/pgTAP/03_high_tests.sql 🧪 Tests +10/-1

Add HIGH severity check tests

• Increment test plan count from 44 to 46
• Add 2 new test cases for shared_buffers At Default check
• Tests verify both function and view return expected check results

testing/pgTAP/03_high_tests.sql


5. testing/pgTAP/04_medium_tests.sql 🧪 Tests +10/-1

Add MEDIUM severity check tests

• Increment test plan count from 14 to 16
• Add 2 new test cases for work_mem At Default check
• Tests verify both function and view return expected check results

testing/pgTAP/04_medium_tests.sql


6. testing/pgTAP/06_info_tests.sql 🧪 Tests +73/-1

Add comprehensive INFO check test coverage

• Increment test plan count from 4 to 20 (16 new tests)
• Add 16 new test cases covering all 8 new INFO-level checks
• Each check tested for both function and view execution
• Tests verify checks return at least 1 row with expected check names

testing/pgTAP/06_info_tests.sql


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Apr 12, 2026

Code Review by Qodo

🐞 Bugs (2)   📘 Rule violations (0)   📎 Requirement gaps (0)   🖥 UI issues (0)   🎨 UX Issues (0)
🐞\ ☼ Reliability (1) ⚙ Maintainability (1)

Grey Divider


Action required

1. View helper missing🐞
Description
view_pgFirstAid.sql and view_pgFirstAid_managed.sql call _pg_firstaid_checkpoint_stats(), but they
do not create it, so executing the view scripts standalone fails with an undefined function error.
This breaks the integration pgTAP runner which installs only the view SQL before running pgTAP
assertions, and contradicts the README guidance that users can install the view by itself.
Code

view_pgFirstAid.sql[R1503-1513]

+-- INFO: Checkpoint statistics (PG15/16: pg_stat_bgwriter, PG17+: pg_stat_checkpointer)
+select
+    'INFO' as severity,
+    'System Health' as category,
+    'Checkpoint Stats' as check_name,
+    'System' as object_name,
+    'Checkpoint activity since stats last reset. Forced checkpoints (checkpoints_req) occur when WAL fills up before the scheduled interval — high ratios suggest max_wal_size may be too small.' as issue_description,
+    _pg_firstaid_checkpoint_stats() as current_value,
+    'If forced checkpoints are consistently above 50% of total, consider increasing max_wal_size. Reset stats with: SELECT pg_stat_reset_shared(''bgwriter'').' as recommended_action,
+    'https://www.postgresql.org/docs/current/monitoring-stats.html#MONITORING-PG-STAT-BGWRITER-VIEW' as documentation_link,
+    5 as severity_order
Evidence
The helper function is defined in pgFirstAid.sql, but both view scripts reference it without
defining it. The pgTAP harness installs only the view SQL before executing pgTAP files, so view
creation will fail in that path; additionally, the README presents the view as a copy/paste install
option.

pgFirstAid.sql[270-300]
view_pgFirstAid.sql[1503-1513]
view_pgFirstAid_managed.sql[1492-1502]
testing/integration/tests/integration/test_pgtap_suite.py[74-86]
README.md[45-56]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`view_pgFirstAid.sql` and `view_pgFirstAid_managed.sql` call `_pg_firstaid_checkpoint_stats()` but don’t define it. The view scripts are used standalone (e.g., pgTAP runner installs only the view file) and are advertised as a standalone install option, so this causes `undefined_function` failures.
## Issue Context
- `_pg_firstaid_checkpoint_stats()` is currently created only in `pgFirstAid.sql`.
- The view scripts already define other helper functions (e.g., `pgfirstaid_pg_stat_statements_checks()`), so adding one more helper in the view scripts is consistent with current structure.
## Fix Focus Areas
- view_pgFirstAid.sql[270-310]
- view_pgFirstAid.sql[1498-1546]
- view_pgFirstAid_managed.sql[270-310]
- view_pgFirstAid_managed.sql[1490-1504]
## Suggested fix
Add the same `CREATE OR REPLACE FUNCTION _pg_firstaid_checkpoint_stats()` definition into both view SQL scripts (before `CREATE VIEW v_pgfirstAid AS ...`), or inline the version-conditional checkpoint-stat selection directly in the view query so the view files remain self-contained.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Env-var defaults removed🐞
Description
TestConfig.from_env now raises ValueError unless PGHOST/PGPORT/PGUSER/PGDATABASE are set, so running
integration tests locally without those env vars will fail at fixture setup instead of using safe
defaults.
Code

testing/integration/src/pgfirstaid_pytest/config.py[R22-36]

+        missing = [name for name in _REQUIRED_ENV_VARS if not os.getenv(name)]
+        if missing:
+            missing_list = ", ".join(missing)
+            raise ValueError(
+                "Missing required PostgreSQL environment variables: "
+                f"{missing_list}. Set the standard PG* connection variables before "
+                "running integration tests."
+            )
+
     return cls(
-            host=os.getenv("PGHOST", "localhost"),
-            port=int(os.getenv("PGPORT", "5432")),
-            user=os.getenv("PGUSER", "postgres"),
+            host=os.environ["PGHOST"],
+            port=int(os.environ["PGPORT"]),
+            user=os.environ["PGUSER"],
         password=os.getenv("PGPASSWORD"),
-            database=os.getenv("PGDATABASE", "postgres"),
+            database=os.environ["PGDATABASE"],
Evidence
The config loader explicitly validates required PG* variables and then reads them via os.environ,
and the integration suite’s session fixture depends on TestConfig.from_env(), so missing env vars
abort the test session early.

testing/integration/src/pgfirstaid_pytest/config.py[5-37]
testing/integration/tests/conftest.py[26-38]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Integration tests fail immediately when standard PG* env vars are not set because `TestConfig.from_env()` raises `ValueError` during fixture setup.
### Issue Context
This is correct for CI, but it makes local runs fail unless developers know to export all PG* variables.
### Fix Focus Areas
- testing/integration/src/pgfirstaid_pytest/config.py[5-37]
- testing/integration/tests/conftest.py[26-38]
### Implementation direction
Choose one:
1) Keep strict env requirements, but change the `config` fixture to `pytest.skip()` with a clear message when vars are missing (so non-integration/local runs don’t hard-fail).
2) Reintroduce safe defaults (e.g., localhost/5432/postgres/postgres) and only enforce requirements in CI or when a marker is enabled.
3) Add pytest configuration (`addopts`) or documentation to ensure integration tests are only run under `-m integration` with env vars set.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Unordered row comparison 🐞
Description
test_view_matches_function_row_order compares rows from pg_firstAid() and v_pgfirstaid without ORDER
BY, so the test can fail even if both return identical sets because row order is not guaranteed.
This makes the integration suite vulnerable to planner changes or view expansion behavior
differences.
Code

testing/integration/tests/integration/test_pgtap_suite.py[R247-268]

+@pytest.mark.integration
+def test_view_matches_function_row_order(
+    db_conn: PgConnection,
+) -> None:
+    with db_conn.cursor() as cur:
+        cur.execute(
+            """
+            SELECT severity, category, check_name
+            FROM pg_firstAid()
+            """
+        )
+        function_rows = [tuple(str(value) for value in row) for row in cur.fetchall()]
+
+        cur.execute(
+            """
+            SELECT severity, category, check_name
+            FROM v_pgfirstaid
+            """
+        )
+        view_rows = [tuple(str(value) for value in row) for row in cur.fetchall()]
+
+    assert function_rows == view_rows
Evidence
The new test fetches rows from both sources without ORDER BY and asserts list equality, which
depends on implicit ordering.

testing/integration/tests/integration/test_pgtap_suite.py[247-268]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A new test compares `fetchall()` results from two SELECT queries that have no `ORDER BY`, then asserts list equality. SQL row order is undefined without an explicit ordering clause, so this assertion can be flaky.
## Issue Context
The test intends to validate parity between function output and view output.
## Fix Focus Areas
- testing/integration/tests/integration/test_pgtap_suite.py[247-268]
## Suggested fix options
1) Make ordering explicit in both queries (recommended):
- Add `ORDER BY severity, category, check_name` to both SELECTs.
2) If you truly want to verify exact presentation order, then select a stable ordering key from both sources (e.g., expose `severity_order` from `pg_firstAid()` or compute it in-test) and `ORDER BY` that key.
3) If you only care about content parity, compare as multisets (e.g., sort both lists in Python before comparing, or compare Counters).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Partial index false positives🐞
Description
The managed view's duplicate-index check matches indexes on the same table by
indkey/indclass/indoption only, so it can flag a partial index and a non-partial index (or two
partial indexes with different predicates) as "duplicates" even though they are not identical. This
can produce misleading HIGH severity results for valid index designs.
Code

view_pgFirstAid_managed.sql[R571-593]

+-- Compare actual index structure (columns, operator class) not string definitions
select
'HIGH' as severity,
'Table Health' as category,
'Duplicate Index' as check_name,
-	quote_ident(i1.schemaname) || '.' || i1.indexname || ' & ' || i2.indexname as object_name,
-	'Multiple indexes with identical or overlapping column sets' as issue_description,
-	'Indexes: ' || i1.indexname || ', ' || i2.indexname as current_value,
+	quote_ident(n1.nspname) || '.' || c1.relname || ': ' || i1.relname || ' & ' || i2.relname as object_name,
+	'Multiple indexes with identical column sets and operator classes' as issue_description,
+	'Indexes: ' || i1.relname || ', ' || i2.relname as current_value,
'Review and consolidate duplicate indexes and focus on keeping the most efficient one' as recommended_action,
'https://www.postgresql.org/docs/current/indexes-multicolumn.html' as documentation_link,
2 as severity_order
from
-	pg_indexes i1
-join pg_indexes i2 on
-	i1.schemaname = i2.schemaname
-	and i1.tablename = i2.tablename
-	and i1.indexname < i2.indexname
-	and i1.indexdef = i2.indexdef
+	pg_index idx1
+join pg_class i1 on idx1.indexrelid = i1.oid
+join pg_class c1 on idx1.indrelid = c1.oid
+join pg_namespace n1 on c1.relnamespace = n1.oid
+join pg_index idx2 on
+	idx1.indrelid = idx2.indrelid
+	and idx1.indexrelid < idx2.indexrelid
+	and idx1.indkey = idx2.indkey
+	and idx1.indclass = idx2.indclass
+	and idx1.indoption = idx2.indoption
+join pg_class i2 on idx2.indexrelid = i2.oid
Evidence
The join predicate declares duplicates based on indkey/indclass/indoption equality but does not
compare predicate (indpred) or expression (indexprs) fields, while the check’s description
claims identical indexes.

view_pgFirstAid_managed.sql[570-595]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The managed view’s duplicate-index detection can misclassify indexes as duplicates because it ignores index predicates/expressions while matching only on `indkey/indclass/indoption`.
## Issue Context
This query is intended to find indexes that are truly duplicates. Partial indexes (different WHERE predicates) and expression indexes should not be treated as duplicates unless their predicate/expression is also identical.
## Fix Focus Areas
- view_pgFirstAid_managed.sql[570-595]
## Suggested fix
In the `pg_index idx1` vs `pg_index idx2` join, extend the equality conditions to include:
- `idx1.indpred IS NOT DISTINCT FROM idx2.indpred`
- `idx1.indexprs IS NOT DISTINCT FROM idx2.indexprs`
Optionally also consider matching on uniqueness/constraint semantics (e.g., `indisunique`, `indisprimary`) if you want to avoid flagging a unique index vs a non-unique index as duplicates.
Also consider quoting identifiers in `object_name` (e.g., `quote_ident(c1.relname)`) for correctness when names require quoting.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

5. Checkpoint helper duplicated 🐞
Description
The new _pg_firstaid_checkpoint_stats() helper is copy-pasted into multiple install entrypoints,
making it easy for future edits to drift and yield inconsistent "Checkpoint Stats" output depending
on which SQL script users run.
Code

pgFirstAid.sql[R270-300]

+-- Helper: returns formatted checkpoint stats compatible with PG15/16 (pg_stat_bgwriter)
+-- and PG17+ (pg_stat_checkpointer, which replaced the checkpoint columns in pg_stat_bgwriter)
+create or replace
+function _pg_firstaid_checkpoint_stats()
+returns text
+language plpgsql
+stable
+as $$
+declare
+    v_timed bigint;
+    v_forced bigint;
+begin
+    if current_setting('server_version_num')::int >= 170000 then
+        select num_timed, num_requested
+        into v_timed, v_forced
+        from pg_stat_checkpointer;
+    else
+        select checkpoints_timed, checkpoints_req
+        into v_timed, v_forced
+        from pg_stat_bgwriter;
+    end if;
+
+    return 'timed: ' || v_timed::text ||
+           ', forced: ' || v_forced::text ||
+           ', forced ratio: ' ||
+           case
+               when v_timed + v_forced = 0 then '0%'
+               else round(100.0 * v_forced / (v_timed + v_forced), 1)::text || '%'
+           end;
+end;
+$$;
Evidence
The identical helper function definition exists in the function installer and both view installers,
so any future change must be kept in sync across all three files to avoid differing behavior.

pgFirstAid.sql[270-300]
view_pgFirstAid.sql[274-303]
view_pgFirstAid_managed.sql[274-303]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_pg_firstaid_checkpoint_stats()` is duplicated in multiple SQL install scripts, which risks future divergence and inconsistent results.
### Issue Context
This helper is used to abstract PG15/16 vs PG17+ checkpoint stat sources. Today the implementations match, but they must remain identical across install paths.
### Fix Focus Areas
- pgFirstAid.sql[270-300]
- view_pgFirstAid.sql[274-303]
- view_pgFirstAid_managed.sql[274-303]
### Implementation direction
- Prefer a single source-of-truth for the function body (e.g., generate the view scripts from a template, or include the helper only in one place and have other scripts depend on it).
- If keeping duplication, add a regression check in tests that asserts the helper function bodies match across the scripts (or at least that the produced `current_value` format matches across variants).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Wasted ORDER BY sort🐞
Description
The new Transaction ID Wraparound Risk check performs ORDER BY age(datfrozenxid) DESC during the
INSERT, but pg_firstAid() later returns rows ordered by severity_order, category, check_name, so
the INSERT-time sort does not affect the output and adds unnecessary work.
Code

pgFirstAid.sql[R1515-1532]

+-- INFO: Transaction ID wraparound risk per database
+insert into health_results
+select
+    'INFO' as severity,
+    'System Health' as category,
+    'Transaction ID Wraparound Risk' as check_name,
+    datname as object_name,
+    'Age of the oldest unfrozen transaction ID in this database. PostgreSQL must freeze XIDs before reaching ~2.1 billion to prevent data loss from wraparound.' as issue_description,
+    datname || ': XID age ' || age(datfrozenxid)::text as current_value,
+    'Run VACUUM FREEZE on databases approaching high XID age. Ensure autovacuum is enabled and not blocked. Monitor databases with age > 500,000,000.' as recommended_action,
+    'https://www.postgresql.org/docs/current/routine-vacuuming.html#VACUUM-FOR-WRAPAROUND' as documentation_link,
+    5 as severity_order
+from
+    pg_database
+where
+    datallowconn = true
+order by
+    age(datfrozenxid) desc;
Evidence
The new check explicitly sorts by XID age during insertion, but the function’s final RETURN QUERY
applies a different ORDER BY that does not include XID age (or even object_name), so the earlier
sort cannot be observed by callers and is therefore wasted.

pgFirstAid.sql[1515-1532]
pgFirstAid.sql[1689-1705]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`pg_firstAid()` inserts wraparound-risk rows with `ORDER BY age(datfrozenxid) DESC`, but the function later returns results using a different final `ORDER BY` (`severity_order, category, check_name`). This means the INSERT-time sort does not affect what users see and is wasted work.
### Issue Context
If you actually want the wraparound-risk rows to be presented highest-risk-first, you need to apply that ordering in the final `RETURN QUERY` ordering; otherwise, the simplest fix is to remove the INSERT-time `ORDER BY`.
### Fix Focus Areas
- pgFirstAid.sql[1515-1532]
- pgFirstAid.sql[1689-1705]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
view_pgFirstAid_managed.sql (1)

1456-1469: Transaction ID Wraparound check is missing ORDER BY clause present in pgFirstAid.sql.

The function version in pgFirstAid.sql (lines 1531-1532) includes order by age(datfrozenxid) desc to present databases with the highest XID age first, which is the most actionable ordering. The view version here is missing this ordering, resulting in inconsistent output between the function and view.

♻️ Proposed fix to add ORDER BY
 from
     pg_database
 where
     datallowconn = true
+order by
+    age(datfrozenxid) desc
 union all
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@view_pgFirstAid_managed.sql` around lines 1456 - 1469, The view that emits
the "Transaction ID Wraparound" check is missing an ORDER BY, causing
inconsistent ordering versus the function; update the SELECT producing the check
(the statement that sets check_name = 'Transaction ID Wraparound' and uses
age(datfrozenxid) in current_value) to append "ORDER BY age(datfrozenxid) DESC"
so databases with the highest XID age appear first, matching the function
behavior.
view_pgFirstAid.sql (1)

1473-1486: Transaction ID Wraparound check is missing ORDER BY clause for consistency with pgFirstAid.sql.

Same issue as in view_pgFirstAid_managed.sql - the function orders by XID age descending, but this view does not.

♻️ Proposed fix to add ORDER BY
 from
     pg_database
 where
     datallowconn = true
+order by
+    age(datfrozenxid) desc
 union all
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@view_pgFirstAid.sql` around lines 1473 - 1486, The "Transaction ID
Wraparound" SELECT in view_pgFirstAid.sql (the query producing check_name =
'Transaction ID Wraparound') lacks an ORDER BY, causing inconsistent ordering
versus view_pgFirstAid_managed.sql; update that SELECT to append an ORDER BY
age(datfrozenxid) DESC (or ORDER BY age(datfrozenxid)::bigint DESC if you need
integer coercion) so results are returned sorted by XID age descending, matching
the existing function/view behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@view_pgFirstAid_managed.sql`:
- Around line 1456-1469: The view that emits the "Transaction ID Wraparound"
check is missing an ORDER BY, causing inconsistent ordering versus the function;
update the SELECT producing the check (the statement that sets check_name =
'Transaction ID Wraparound' and uses age(datfrozenxid) in current_value) to
append "ORDER BY age(datfrozenxid) DESC" so databases with the highest XID age
appear first, matching the function behavior.

In `@view_pgFirstAid.sql`:
- Around line 1473-1486: The "Transaction ID Wraparound" SELECT in
view_pgFirstAid.sql (the query producing check_name = 'Transaction ID
Wraparound') lacks an ORDER BY, causing inconsistent ordering versus
view_pgFirstAid_managed.sql; update that SELECT to append an ORDER BY
age(datfrozenxid) DESC (or ORDER BY age(datfrozenxid)::bigint DESC if you need
integer coercion) so results are returned sorted by XID age descending, matching
the existing function/view behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 496f3877-7fab-4682-b8ec-56300b17f597

📥 Commits

Reviewing files that changed from the base of the PR and between 5e77b75 and 429769b.

📒 Files selected for processing (7)
  • .gitignore
  • pgFirstAid.sql
  • testing/pgTAP/03_high_tests.sql
  • testing/pgTAP/04_medium_tests.sql
  • testing/pgTAP/06_info_tests.sql
  • view_pgFirstAid.sql
  • view_pgFirstAid_managed.sql

@randoneering
Copy link
Copy Markdown
Owner Author

Testing out different review agents as I am the only one contributing at the moment. I will only have one moving forward. Sorry for the traffic here!

…bility

PostgreSQL 17 moved checkpoint columns from pg_stat_bgwriter to the new
pg_stat_checkpointer view with renamed columns (checkpoints_timed → num_timed,
checkpoints_req → num_requested). Adds _pg_firstaid_checkpoint_stats() helper
function using PL/pgSQL lazy compilation to branch on server version, so both
the main function and views work correctly on PG15/16 and PG17+.
@randoneering
Copy link
Copy Markdown
Owner Author

/agentic_review
Triggering review by Qodo

@greptileai review this pr for me

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects Bot commented Apr 12, 2026

Persistent review updated to latest commit 9af5018

Comment thread view_pgFirstAid.sql
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 12, 2026

Greptile Summary

This PR adds 10 new health checks covering memory configuration parameters (shared_buffers, work_mem, effective_cache_size, maintenance_work_mem), transaction ID wraparound risk per database, checkpoint statistics, server role, and connection utilization — all using built-in system catalogs with no extension dependencies. It also migrates the Python test harness from psycopg3 to psycopg2 and improves the config.py validation for required env vars.

  • P1 — standalone view install is broken: _pg_firstaid_checkpoint_stats() is called in the Checkpoint Stats check inside both view_pgFirstAid.sql and view_pgFirstAid_managed.sql, but the function is only defined in pgFirstAid.sql. PostgreSQL validates function references at CREATE VIEW time, so executing either view file without first running pgFirstAid.sql raises ERROR: function _pg_firstaid_checkpoint_stats() does not exist. The function definition must be added to both view files.
  • P2 — reset hint incorrect on PG17+: The recommended_action for the Checkpoint Stats check advises pg_stat_reset_shared('bgwriter'), but on PG17+ checkpoint stats moved to pg_stat_checkpointer and require pg_stat_reset_shared('checkpointer') to reset.

Confidence Score: 4/5

Do not merge until _pg_firstaid_checkpoint_stats() is defined in both view files; otherwise standalone view installation is broken.

One P1 defect blocks the stated standalone-view deployment model — both view files call a function that only exists in pgFirstAid.sql. All other changes (new health checks, psycopg2 migration, test improvements) are well-structured with good test coverage.

view_pgFirstAid.sql and view_pgFirstAid_managed.sql both need the _pg_firstaid_checkpoint_stats() function definition added before the CREATE VIEW statement.

Important Files Changed

Filename Overview
pgFirstAid.sql Adds _pg_firstaid_checkpoint_stats() helper function and 10 new INFO/HIGH/MEDIUM health checks; the reset hint in the checkpoint check's recommended_action is PG17-inaccurate.
view_pgFirstAid.sql Adds the same 10 new checks to the view but omits the _pg_firstaid_checkpoint_stats() function definition, breaking standalone installation.
view_pgFirstAid_managed.sql Same issue as view_pgFirstAid.sql: calls _pg_firstaid_checkpoint_stats() without defining it; also includes an improved duplicate-index detection query using pg_index system catalogs.
testing/pgTAP/06_info_tests.sql Expands INFO test plan from 4 to 20; adds presence assertions for all 8 new INFO checks in both the function and view.
testing/integration/tests/integration/test_pgtap_suite.py Adds teardown resilience test and new integration assertions for wraparound formatting and view/function row-order parity; migrates from psycopg3 to psycopg2.
testing/integration/src/pgfirstaid_pytest/db.py Migrates from psycopg3 to psycopg2; converts connect() to a context manager that ensures connection cleanup.
testing/pgTAP/99_teardown.sql Correctly adds DROP statements for the new view and _pg_firstaid_checkpoint_stats() helper function.
testing/integration/src/pgfirstaid_pytest/config.py Adds explicit validation for required PG connection env vars, giving a clear error instead of silently using defaults.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[pg_firstAid / v_pgfirstaid] --> B[Memory Settings]
    A --> C[XID Wraparound]
    A --> D[Checkpoint Stats]
    A --> E[Server Role]
    A --> F[Connection Utilization]

    B --> B1["current_setting('shared_buffers')"]
    B --> B2["current_setting('work_mem')"]
    B --> B3["current_setting('effective_cache_size')"]
    B --> B4["current_setting('maintenance_work_mem')"]

    C --> C1[pg_database\ndatfrozenxid, datallowconn]

    D --> D2{server_version_num >= 170000?}
    D2 -- Yes --> D3[pg_stat_checkpointer\nnum_timed, num_requested]
    D2 -- No --> D4[pg_stat_bgwriter\ncheckpoints_timed, checkpoints_req]
    D3 & D4 --> DF["_pg_firstaid_checkpoint_stats()"]

    E --> E1["pg_is_in_recovery()"]
    F --> F1[pg_stat_activity\ncount all connections]
Loading

Fix All in Claude Code

Reviews (1): Last reviewed commit: "fix: resolve integration and pgTap issue..." | Re-trigger Greptile

Comment thread view_pgFirstAid.sql
Comment thread pgFirstAid.sql Outdated
@randoneering
Copy link
Copy Markdown
Owner Author

Unfortunately, I can no longer test against GCP or Azure as I no longer have credits. These will remain untested until I can fund some credits myself or someone from the community can validate against Azure and GCP. Thank you for understanding.

@randoneering
Copy link
Copy Markdown
Owner Author

randoneering commented Apr 13, 2026

/agentic_review
Triggering review by Qodo

@greptileai review this pr for me

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects Bot commented Apr 13, 2026

Persistent review updated to latest commit 78880c5

@randoneering
Copy link
Copy Markdown
Owner Author

/agentic_review
Triggering review by Qodo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects Bot commented Apr 13, 2026

Persistent review updated to latest commit 998dbf2

@randoneering randoneering merged commit 5a48b09 into main Apr 13, 2026
4 checks passed
@randoneering randoneering deleted the feature/system_info_checks branch April 13, 2026 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant