pal-e-docs

Scope Review: #86 pg_stat_statements + query analysis dashboard

review-1309-2026-06-04 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Board item #1309 | Forgejo issue ldraney/landscaping-assistant#86 | Reviewed 2026-06-04 by agent-qa

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- Child of #43, references Phase 2 of observability roadmap
  • [ ] Repo -- Lists forgejo_admin/pal-e-platform and forgejo_admin/pal-e-deployments. Both repos are actually ldraney/pal-e-platform and ldraney/pal-e-deployments. Verified via Forgejo API. Wrong owner will cause agent lookup failures.
  • [x] User Story -- Clear "As a platform operator / I want query-level database visibility / So that I can identify slow queries"
  • [x] Context -- Good motivation, explains the gap (PodMonitors give connection-level but not query-level visibility)
  • [ ] File Targets -- Two of three targets lack concrete file paths. Only terraform/dashboards/postgres-query-analysis.json is specific. The CNPG cluster spec and Postgres config targets say "(pal-e-deployments)" without naming the actual file. See File Targets section below.
  • [x] Acceptance Criteria -- 4 criteria, all checkboxed. One has an internal contradiction (see below).
  • [x] Test Expectations -- 3 expectations: SQL query check, dashboard load, Loki log check
  • [ ] Constraints -- Contains a contradiction. States "Must not cause CNPG cluster downtime" but also acknowledges "pg_stat_statements requires restart." See Acceptance Criteria section below.
  • [x] Checklist -- PRs for both repos listed
  • [x] Related -- References project-pal-e-platform and parent #43

Traceability

  • [x] story:observability -- present on board item labels
  • [x] arch:platform, arch:k8s-deploy -- present on board item labels
  • [x] Forgejo issue #86 -- exists, open, linked from board item
  • [x] Parent #43 -- exists, open, confirmed as observability tracking issue

File Targets

  • [x] terraform/dashboards/postgres-query-analysis.json (pal-e-platform) -- NEW file. Parent directory verified: terraform/dashboards/ exists with 8 dashboard JSON files (e.g., landscaping-assistant-golden-signals.json, dora-dashboard.json). Naming convention is consistent.
  • [ ] "CNPG cluster spec (pal-e-deployments)" -- NO PATH SPECIFIED. The pal-e-postgres CNPG Cluster manifest is NOT in pal-e-platform (confirmed via code search -- zero results for pg_stat_statements, shared_preload_libraries, or any Cluster kind in the database module). It must be in pal-e-deployments, but that repo is private and the exact path (likely a Kustomize base or overlay YAML) is not stated. An agent cannot act on this without the path.
  • [ ] "CNPG Postgres config (pal-e-deployments)" -- NO PATH SPECIFIED. Same problem. The log_min_duration_statement parameter would go in the same Cluster manifest's postgresql.parameters section, but no file is named. Additionally, no threshold value is specified for log_min_duration_statement.
Assessment: Targets are NOT specific enough for an agent to act without guessing. The Grafana dashboard target is good. The two pal-e-deployments targets are too vague.

Repo Placement

This issue is filed in ldraney/landscaping-assistant (the Rails app), but ALL file changes are in pal-e-platform and pal-e-deployments. Zero files in landscaping-assistant are modified.
This is acceptable because the parent tracking issue #43 lives in landscaping-assistant and all observability children (#85-#90) are filed here for consistency. However, this means the issue's Repo section is the ONLY way an agent knows which repos to target -- making the wrong owner (forgejo_admin/) a higher-severity problem than usual.
Multi-repo scope: YES (2 repos). The checklist correctly lists separate PRs for each repo. This is structured appropriately as a single coordinated ticket rather than needing child issues, since the changes are tightly coupled (extension enable + dashboard that reads the metrics).

Dependencies

  • [x] CNPG operator deployed -- satisfied (confirmed in terraform/modules/database/main.tf, CNPG chart v0.27.1)
  • [x] PodMonitor infrastructure -- satisfied (monitoring module sets podMonitorSelectorNilUsesHelmValues = false for cluster-wide discovery; CNPG database module enables podMonitorEnabled = true)
  • [x] Grafana sidecar pattern -- satisfied (grafana_dashboard: "1" label used across 8 dashboards in monitoring module)
  • [x] Prometheus scraping -- satisfied (existing PodMonitors working)
  • [x] Phase 1 (Tempo/OTel, #85) -- NOT a dependency. The ticket correctly does not depend on it. However, the roadmap notes that log-trace correlation (slow query -> trace) will only work after Phase 1 ships. This is informational, not blocking.
No unresolved dependencies that would block execution.

Acceptance Criteria

  • AC1: pg_stat_statements extension enabled -- Testable. SELECT * FROM pg_stat_statements LIMIT 1 is a direct verification. Note: enabling the extension requires both (a) adding to shared_preload_libraries in the CNPG Cluster spec AND (b) running CREATE EXTENSION pg_stat_statements. The ticket does not mention step (b). For an existing cluster, this needs a one-time SQL command or CNPG's managed extension list.
  • AC2: Grafana dashboard shows top queries -- Testable. Dashboard load with real data is verifiable.
  • AC3: Slow queries logged and searchable in Loki -- Testable. But the log_min_duration_statement threshold value is not specified. Agent will have to guess a value (common defaults: 500ms, 1000ms).
  • AC4: "No CNPG cluster restart required (or planned maintenance window if needed)" -- Contradicts the Constraints section which says "pg_stat_statements requires restart." The parenthetical escape hatch "(or planned maintenance window if needed)" is ambiguous. This should be rewritten to state the reality: a restart IS required, and either (a) CNPG handles it as a rolling restart (multi-instance) with zero downtime, or (b) brief downtime is accepted for single-instance clusters during a maintenance window.

Blast Radius

  • Files touched: ~3 files across 2 repos (CNPG Cluster manifest, possibly a custom queries ConfigMap, Grafana dashboard JSON)
  • Services affected: pal-e-postgres (restart required), Prometheus (new metrics), Grafana (new dashboard)
  • What could break: The Postgres restart is the main risk. If pal-e-postgres is single-instance (like the Woodpecker cluster), there will be brief downtime affecting all services using this database. The Grafana dashboard and Prometheus scraping are additive and low-risk.
  • Rollback: Straightforward. Remove shared_preload_libraries entry and restart (same risk), delete dashboard ConfigMap. CNPG handles rollback gracefully.

Decomposition Assessment

Three-thing limit: 3 discrete changes (enable extension, create dashboard, configure slow query logging). Fits within the limit.
Five-minute rule: The Grafana dashboard JSON will be the bulk of the work (PromQL queries, panel layout). The CNPG config changes are small. Overall scope is reasonable for a single ticket -- estimated 3 story points on the board matches.
Parallelization: The dashboard JSON could be developed independently of the CNPG config, but it needs real pg_stat_statements data to verify. Sequential execution is appropriate.

Recommendation

  • Fix repo owner (BLOCKER): Change forgejo_admin/pal-e-platform to ldraney/pal-e-platform and forgejo_admin/pal-e-deployments to ldraney/pal-e-deployments.
  • Add concrete file paths for pal-e-deployments targets (BLOCKER): Specify the exact path to the CNPG Cluster manifest (e.g., bases/postgres/cluster.yaml or overlays/production/cnpg-cluster.yaml -- whatever the actual path is).
  • Resolve restart contradiction: Rewrite AC4 and Constraints to explicitly state: (a) how many instances pal-e-postgres has, (b) whether brief downtime is acceptable, (c) whether a maintenance window is needed.
  • Specify log_min_duration_statement threshold: Add a concrete value (e.g., 500ms or 1000ms) so the agent does not have to guess.
  • Add CREATE EXTENSION step: Note in File Targets or Constraints that the extension must be created after enabling shared_preload_libraries, and specify the mechanism (CNPG managed extensions list, postInitSQL, or manual SQL).
  • Clarify pg_stat_statements metric exposure: Specify whether CNPG's default metrics exporter covers pg_stat_statements after the extension is loaded, or whether a custom queries ConfigMap is needed.