Merge pull request #25 from randoneering/bugfix/pgTAP

pgTAP bug fix and created GH Action to test with PRs

Author: randoneering

.github/workflows/integration-pg-matrix.yml

@@ -0,0 +1,93 @@
+name: Python + pgTAP Integration (PG15-PG18)
+
+on:
+  pull_request:
+    paths:
+      - pgFirstAid.sql
+      - view_pgFirstAid.sql
+      - view_pgFirstAid_managed.sql
+      - testing/integration/**
+      - testing/pgTAP/**
+      - .github/workflows/integration-pg-matrix.yml
+  workflow_dispatch:
+
+concurrency:
+  group: integration-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  integration:
+    if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.fork == false
+    runs-on: [self-hosted, linux, pgfirstaid-ci]
+    permissions:
+      contents: read
+    strategy:
+      fail-fast: false
+      matrix:
+        postgres_version: ["15", "16", "17", "18"]
+
+    name: PG${{ matrix.postgres_version }} integration
+
+    defaults:
+      run:
+        working-directory: testing/integration
+
+    env:
+      PGHOST: ${{ matrix.postgres_version == '15' && secrets.PG15_HOST || matrix.postgres_version == '16' && secrets.PG16_HOST || matrix.postgres_version == '17' && secrets.PG17_HOST || secrets.PG18_HOST }}
+      PGPORT: ${{ matrix.postgres_version == '15' && secrets.PG15_PORT || matrix.postgres_version == '16' && secrets.PG16_PORT || matrix.postgres_version == '17' && secrets.PG17_PORT || secrets.PG18_PORT }}
+      PGUSER: ${{ matrix.postgres_version == '15' && secrets.PG15_USER || matrix.postgres_version == '16' && secrets.PG16_USER || matrix.postgres_version == '17' && secrets.PG17_USER || secrets.PG18_USER }}
+      PGPASSWORD: ${{ matrix.postgres_version == '15' && secrets.PG15_PASSWORD || matrix.postgres_version == '16' && secrets.PG16_PASSWORD || matrix.postgres_version == '17' && secrets.PG17_PASSWORD || secrets.PG18_PASSWORD }}
+      PGDATABASE: ${{ matrix.postgres_version == '15' && secrets.PG15_DATABASE || matrix.postgres_version == '16' && secrets.PG16_DATABASE || matrix.postgres_version == '17' && secrets.PG17_DATABASE || secrets.PG18_DATABASE }}
+      PGSSLMODE: require
+      PGFA_TEST_VIEW_MODE: managed
+      PGFA_TEST_ACTIVE_CONN_TARGET: "52"
+      PGFA_TEST_ACTIVE_CONN_SLEEP_SECONDS: "20"
+      PGFA_TEST_WAIT_TIMEOUT_SECONDS: "45"
+
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+
+      - name: Setup Python
+        uses: actions/setup-python@v5
+        with:
+          python-version: "3.14"
+
+      - name: Install uv
+        uses: astral-sh/setup-uv@v4
+
+      - name: Validate required PG env vars
+        run: |
+          missing=0
+          for var in PGHOST PGPORT PGUSER PGPASSWORD PGDATABASE; do
+            if [ -z "${!var}" ]; then
+              echo "::error::Missing required secret/env: ${var} for PG${{ matrix.postgres_version }}"
+              missing=1
+            fi
+          done
+          if [ "$missing" -ne 0 ]; then
+            exit 1
+          fi
+
+      - name: Verify PostgreSQL client is installed
+        run: |
+          if ! command -v psql >/dev/null 2>&1; then
+            echo "::error::psql not found on runner. Install postgresql-client on the self-hosted VM."
+            exit 1
+          fi
+          psql --version
+
+      - name: Sync dependencies
+        run: uv sync
+
+      - name: Install pgFirstAid function
+        run: |
+          psql -v ON_ERROR_STOP=1 -f ../../pgFirstAid.sql
+
+      - name: Recreate managed view only
+        run: |
+          psql -v ON_ERROR_STOP=1 -c "DROP VIEW IF EXISTS v_pgfirstaid"
+          psql -v ON_ERROR_STOP=1 -f ../../view_pgFirstAid_managed.sql
+
+      - name: Run integration tests
+        run: uv run python -m pytest tests/integration -m integration

.gitignore

@@ -11,3 +11,21 @@ terraform.tfstate.backup
 *.pem
 *.key
 credentials.json
+.pgpass
+pgpass
+# AI dirs
+.claude
+.agent
+.copilot
+
+# AI Files
+CLAUDE.md
+
+# Python
+__pycache__/
+.pytest_cache/
+*.pyc
+
+# Test logs
+*.log
+testing/pgTAP/logs/

pgFirstAid.sql

@@ -88,12 +88,8 @@ with q as (
 		database,
 		restart_lsn,
 		case
-			when invalidation_reason is not null then 'invalid'
-			else
-          case
-				when active is true then 'active'
-				else 'inactive'
-			end
+			when active is true then 'active'
+			else 'inactive'
 		end as "status",
 		pg_size_pretty(
         pg_wal_lsn_diff(
@@ -326,28 +322,34 @@ order by
 from
 	ts;
 -- HIGH: Duplicate or redundant indexes
+-- Compare actual index structure (columns, operator class) not string definitions
     insert
 	into
 	health_results
     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  -- same table
+	and idx1.indexrelid < idx2.indexrelid  -- avoid duplicates
+	and idx1.indkey = idx2.indkey  -- same columns
+	and idx1.indclass = idx2.indclass  -- same operator classes
+	and idx1.indoption = idx2.indoption  -- same options
+join pg_class i2 on idx2.indexrelid = i2.oid
 where
-	i1.schemaname not like all(array['information_schema', 'pg_catalog', 'pg_toast', 'pg_temp%']);
+	n1.nspname not like all(array['information_schema', 'pg_catalog', 'pg_toast', 'pg_temp%']);
 -- HIGH: Table with more than 200 columns
 with cc as (
 select
@@ -507,12 +509,8 @@ with q as (
 		database,
 		restart_lsn,
 		case
-			when invalidation_reason is not null then 'invalid'
-			else
-      case
-				when active is true then 'active'
-				else 'inactive'
-			end
+			when active is true then 'active'
+			else 'inactive'
 		end as "status",
 		pg_size_pretty(
     pg_wal_lsn_diff(
@@ -666,8 +664,9 @@ from
         ) as object_name,
 	'The following query has been running for more than 5 minutes. Might be helpful to see if this is expected behavior' as issue_description,
 	query as current_value,
-	'Review query using EXPLAIN ANALYZE to identify any bottlenecks, such as full table scans, missing indexes, etc' as recommendation_action,
-	'https://www.postgresql.org/docs/current/using-explain.html' as documentation_link
+	'Review query using EXPLAIN ANALYZE to identify any bottlenecks, such as full table scans, missing indexes, etc' as recommended_action,
+	'https://www.postgresql.org/docs/current/using-explain.html' as documentation_link,
+	3 as severity_order
 from
 	pg_stat_activity pgs
 where
@@ -814,8 +813,9 @@ select
 from
 	pg_stat_user_tables
 where
-	coalesce(seq_scan, 0) + coalesce(idx_scan, 0) = 0
-		and coalesce(n_tup_ins, 0) + coalesce(n_tup_upd, 0) + coalesce(n_tup_del, 0) = 0
+	schemaname not like all(array['pg_catalog', 'information_schema', 'pg_toast', 'pg_temp%'])
+	and coalesce(seq_scan, 0) + coalesce(idx_scan, 0) = 0
+	and coalesce(n_tup_ins, 0) + coalesce(n_tup_upd, 0) + coalesce(n_tup_del, 0) = 0
 )
 insert
 	into

testing/integration/README.md

@@ -0,0 +1,44 @@
+# Python Integration/Validation Tests for pgFirstAid
+
+This test harness runs pgFirstAid integration tests with `pytest`. One of the things I wanted to make sure this project does is validate each health check. While these tests might not be perfect, but the aim is to reduce the chances of a health check making its way into `main` and it fails to do its job.
+
+
+It uses:
+
+- Integration tests using (`psycopg`) for live runtime behavior(for checks looking for active connections)
+- Execution of the pgTAP SQL suite sing Python
+
+## What is covered
+
+- pgTAP assertions grouped by severity (`testing/pgTAP/01_*.sql` to `06_*.sql`)
+- Python integration scenarios that need concurrent sessions and timing control
+- Function/view parity assertions
+- A coverage guard test that ensures every `check_name` in `pgFirstAid.sql` is
+  referenced by at least one pgTAP assertion
+
+## Configure connection
+
+Set standard PostgreSQL environment variables before running tests:
+
+```bash
+export PGHOST=your-host
+export PGPORT=5432
+export PGUSER=your-user
+export PGPASSWORD=your-password
+export PGDATABASE=your-database
+```
+
+Optional tuning variables:
+
+```bash
+export PGFA_TEST_ACTIVE_CONN_TARGET=52
+export PGFA_TEST_ACTIVE_CONN_SLEEP_SECONDS=20
+export PGFA_TEST_WAIT_TIMEOUT_SECONDS=45
+```
+
+## Run tests
+
+```bash
+uv sync
+uv run pytest tests/integration -m integration
+```

testing/integration/pyproject.toml

@@ -0,0 +1,19 @@
+[project]
+name = "pgfirstaid-python-tests"
+version = "0.1.0"
+description = "Python integration harness for pgFirstAid health checks"
+requires-python = ">=3.11"
+dependencies = [
+  "psycopg[binary]>=3.2.0",
+  "pytest>=8.3.0",
+]
+
+[tool.pytest.ini_options]
+testpaths = ["tests"]
+markers = [
+  "integration: integration tests against a live PostgreSQL instance",
+  "slow: tests that open many connections or sleep",
+]
+
+[tool.ruff]
+line-length = 88

testing/integration/src/pgfirstaid_pytest/__init__.py

@@ -0,0 +1,4 @@
+from .config import TestConfig
+from .db import connect, execute_sql_file, wait_for_sql_true
+
+__all__ = ["TestConfig", "connect", "execute_sql_file", "wait_for_sql_true"]

testing/integration/src/pgfirstaid_pytest/config.py

@@ -0,0 +1,31 @@
+from dataclasses import dataclass
+import os
+
+
+@dataclass(frozen=True)
+class TestConfig:
+    host: str
+    port: int
+    user: str
+    password: str | None
+    database: str
+    sslmode: str | None
+    active_conn_target: int
+    active_conn_sleep_seconds: int
+    wait_timeout_seconds: int
+
+    @classmethod
+    def from_env(cls) -> "TestConfig":
+        return cls(
+            host=os.getenv("PGHOST", "localhost"),
+            port=int(os.getenv("PGPORT", "5432")),
+            user=os.getenv("PGUSER", "postgres"),
+            password=os.getenv("PGPASSWORD"),
+            database=os.getenv("PGDATABASE", "postgres"),
+            sslmode=os.getenv("PGSSLMODE"),
+            active_conn_target=int(os.getenv("PGFA_TEST_ACTIVE_CONN_TARGET", "52")),
+            active_conn_sleep_seconds=int(
+                os.getenv("PGFA_TEST_ACTIVE_CONN_SLEEP_SECONDS", "20")
+            ),
+            wait_timeout_seconds=int(os.getenv("PGFA_TEST_WAIT_TIMEOUT_SECONDS", "45")),
+        )

testing/integration/src/pgfirstaid_pytest/db.py

@@ -0,0 +1,54 @@
+from pathlib import Path
+import time
+from typing import Any
+
+import psycopg
+
+from .config import TestConfig
+
+
+def connect(
+    config: TestConfig,
+    *,
+    autocommit: bool = True,
+    application_name: str | None = None,
+) -> psycopg.Connection[Any]:
+    kwargs: dict[str, Any] = {
+        "host": config.host,
+        "port": config.port,
+        "user": config.user,
+        "dbname": config.database,
+        "autocommit": autocommit,
+    }
+    if config.password is not None:
+        kwargs["password"] = config.password
+    if config.sslmode is not None:
+        kwargs["sslmode"] = config.sslmode
+    if application_name is not None:
+        kwargs["application_name"] = application_name
+    return psycopg.connect(**kwargs)
+
+
+def execute_sql_file(conn: psycopg.Connection[Any], file_path: Path) -> None:
+    sql_text = file_path.read_text(encoding="utf-8")
+    with conn.cursor() as cur:
+        cur.execute(sql_text)
+
+
+def wait_for_sql_true(
+    conn: psycopg.Connection[Any],
+    sql: str,
+    params: tuple[Any, ...] | None = None,
+    *,
+    timeout_seconds: int,
+    interval_seconds: float = 0.5,
+) -> bool:
+    start = time.monotonic()
+    with conn.cursor() as cur:
+        while time.monotonic() - start < timeout_seconds:
+            cur.execute(sql, params)
+            row = cur.fetchone()
+            if row and bool(row[0]):
+                return True
+            time.sleep(interval_seconds)
+    return False