Conversation
📝 WalkthroughWalkthroughThis PR updates PostgreSQL collectors and documentation to use richer catalog/stat views (adds bloat/pgstattuple/sequences, switches to pg_available_extension_versions, adds stat_replication_slots and stat_ssl), adds a unit test validating query contents, updates collector expectations, and separately broadens the Linux read_ahead system task to probe more device globs. ChangesPostgreSQL Data Collection Expansion
System Task: read_ahead probe
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 0 duplication
Metric Results Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
vyruss
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
radar_test.go (1)
532-559: ⚡ Quick winExpand coverage checks for changed
available_extensionsand age fields.This test is a great guardrail; add assertions for the
available_extensionssource token and the new computed database age aliases so future query edits don’t silently regress those additions.🔧 Suggested test additions
checks := []struct { taskList string taskName string mustContain []string }{ + {"postgres", "available_extensions", []string{ + "pg_available_extension_versions", "installed", "trusted", "requires", + }}, {"postgres", "databases", []string{ "datfrozenxid", "datminmxid", "datconnlimit", - "datistemplate", "datallowconn", + "datistemplate", "datallowconn", "frozenxid_age", "minmxid_age", }},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@radar_test.go` around lines 532 - 559, Update the test expectations so the databases coverage includes the new available_extensions source token and the computed database age aliases: add "available_extensions" to the {"postgres","databases",...} expected fields and add the age-related alias names used by the query (the computed database age column(s) labeled in the query, e.g., "age" and any alias variant) to the appropriate databases/perDB expectations (look for the {"postgres","databases",...} and {"perDB","tables"/"indexes"/...} entries and append the new field names).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@radar_test.go`:
- Around line 532-559: Update the test expectations so the databases coverage
includes the new available_extensions source token and the computed database age
aliases: add "available_extensions" to the {"postgres","databases",...} expected
fields and add the age-related alias names used by the query (the computed
database age column(s) labeled in the query, e.g., "age" and any alias variant)
to the appropriate databases/perDB expectations (look for the
{"postgres","databases",...} and {"perDB","tables"/"indexes"/...} entries and
append the new field names).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9e9d5c8-0d3c-4b73-a361-42f1a8dac090
📒 Files selected for processing (7)
DATA.mdREADME.mddocs/data.mddocs/index.mdpostgres_tasks.gopostgres_test.goradar_test.go
vyruss
changed the title
vyruss
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (3)
system_tasks_linux.go (1)
338-338: ⚡ Quick winConsider adding
/dev/md*(software RAID) to the device glob list.Enterprise PostgreSQL deployments frequently sit on software RAID arrays (
mdadm), andblockdev --getrais a legitimate read-ahead tuning surface for those devices. The same[ -b "$d" ]guard already handles no-match silently.♻️ Proposed addition
-Args: []string{"-c", "for d in /dev/sd* /dev/nvme* /dev/vd* /dev/xvd*; do [ -b \"$d\" ] && ra=$(blockdev --getra \"$d\" 2>/dev/null) && [ -n \"$ra\" ] && echo \"$d: $ra\"; done; exit 0"}, +Args: []string{"-c", "for d in /dev/sd* /dev/nvme* /dev/vd* /dev/xvd* /dev/md*; do [ -b \"$d\" ] && ra=$(blockdev --getra \"$d\" 2>/dev/null) && [ -n \"$ra\" ] && echo \"$d: $ra\"; done; exit 0"},Note:
/dev/dm-*(LVM/device-mapper) read-ahead is technically valid too, but its behaviour at the DM layer is less commonly tuned and the output can be voluminous on systems with many LVs — leaving it out is a reasonable call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system_tasks_linux.go` at line 338, The device glob used in the command assigned to Args currently iterates /dev/sd*, /dev/nvme*, /dev/vd*, /dev/xvd* but omits software RAID devices; update the command string (the Args value in system_tasks_linux.go where Args: []string{"-c", "..."} is defined) to include /dev/md* in the glob (e.g. add /dev/md* to the list) so blockdev --getra will also query mdadm devices while keeping the existing [ -b "$d" ] guard and the rest of the pipeline intact.radar_test.go (2)
532-559: ⚡ Quick win
available_extensionsis missing from the coverage checks despite being reworked in this PR.The test's stated goal is to "catch accidental column removal during future edits." The
available_extensionstask was explicitly reworked in this PR to usepg_available_extension_versions, addinginstalled,trusted,relocatable,requires, andsuperuser— yet none of those new columns are guarded here.✅ Suggested addition
{"postgres", "databases", []string{ "datfrozenxid", "datminmxid", "datconnlimit", "datistemplate", "datallowconn", }}, + {"postgres", "available_extensions", []string{ + "pg_available_extension_versions", "installed", "trusted", + "relocatable", "requires", "superuser", + }},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@radar_test.go` around lines 532 - 559, The test coverage list is missing the reworked available_extensions check; add a new entry for the "available_extensions" task in the coverage slice (similar to other {"postgres", "...", []string{...}} entries) and include the new columns added in this PR: "installed", "trusted", "relocatable", "requires", and "superuser" (alongside any existing name/version columns used by available_extensions) so the test will catch accidental removals; locate the slice in radar_test.go where other {"postgres", "...", []string{...}} entries are defined and append the {"postgres", "available_extensions", []string{ "installed", "trusted", "relocatable", "requires", "superuser", /* include existing columns like name/version as appropriate */ }} entry.
573-578: ⚡ Quick winAdd a
defaultcase to fail clearly on unrecognizedtaskListvalues.Without a
default, a typo intaskList(e.g.,"Postgres"instead of"postgres") leavestaskasnil, and the failure message at line 580 saystask "X" not found in Postgres tasks— suggesting the task itself is missing rather than that the list identifier is wrong.🛡️ Proposed fix
switch c.taskList { case "postgres": task = taskByName(postgresQueryTasks, c.taskName) case "perDB": task = taskByName(perDatabaseQueryTasks, c.taskName) + default: + t.Errorf("unknown taskList %q for task %q", c.taskList, c.taskName) + continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@radar_test.go` around lines 573 - 578, The switch on c.taskList that assigns task (using taskByName with postgresQueryTasks or perDatabaseQueryTasks) needs a default branch to fail fast when c.taskList is unrecognized; add a default case in that switch which calls the test failure helper (e.g., t.Fatalf or equivalent) with a clear message stating the invalid taskList value and the allowed options so typos (like "Postgres") surface as an invalid list rather than a missing task; ensure the error references c.taskList and the expected names to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@radar_test.go`:
- Around line 532-559: The test coverage list is missing the reworked
available_extensions check; add a new entry for the "available_extensions" task
in the coverage slice (similar to other {"postgres", "...", []string{...}}
entries) and include the new columns added in this PR: "installed", "trusted",
"relocatable", "requires", and "superuser" (alongside any existing name/version
columns used by available_extensions) so the test will catch accidental
removals; locate the slice in radar_test.go where other {"postgres", "...",
[]string{...}} entries are defined and append the {"postgres",
"available_extensions", []string{ "installed", "trusted", "relocatable",
"requires", "superuser", /* include existing columns like name/version as
appropriate */ }} entry.
- Around line 573-578: The switch on c.taskList that assigns task (using
taskByName with postgresQueryTasks or perDatabaseQueryTasks) needs a default
branch to fail fast when c.taskList is unrecognized; add a default case in that
switch which calls the test failure helper (e.g., t.Fatalf or equivalent) with a
clear message stating the invalid taskList value and the allowed options so
typos (like "Postgres") surface as an invalid list rather than a missing task;
ensure the error references c.taskList and the expected names to aid debugging.
In `@system_tasks_linux.go`:
- Line 338: The device glob used in the command assigned to Args currently
iterates /dev/sd*, /dev/nvme*, /dev/vd*, /dev/xvd* but omits software RAID
devices; update the command string (the Args value in system_tasks_linux.go
where Args: []string{"-c", "..."} is defined) to include /dev/md* in the glob
(e.g. add /dev/md* to the list) so blockdev --getra will also query mdadm
devices while keeping the existing [ -b "$d" ] guard and the rest of the
pipeline intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df0d4cf9-354b-4f46-8328-2672125843fe
📒 Files selected for processing (5)
DATA.mddocs/data.mdpostgres_tasks.goradar_test.gosystem_tasks_linux.go
🚧 Files skipped from review as they are similar to previous changes (2)
- DATA.md
- postgres_tasks.go
vyruss
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/data.md (1)
49-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
read_ahead.outsource description not updated to reflect the bug fix.Line 93 (unchanged) still documents:
| `system/read_ahead.out` | `blockdev --getra /dev/*` | ...The PR explicitly fixes
read_aheadto also probe/dev/vd*(virtio) and/dev/xvd*(Xen) in addition to the original pattern. The source description should reflect the multi-pattern probe, e.g.`blockdev --getra` (sd/vd/xvd/nvme)or similar, so readers understand virtio and Xen hosts are covered.📝 Suggested description update
-| `system/read_ahead.out` | `blockdev --getra /dev/*` | Block device read-ahead settings | +| `system/read_ahead.out` | `blockdev --getra` (sd/vd/xvd/nvme) | Block device read-ahead settings per device |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/data.md` around lines 49 - 112, Update the table entry for system/read_ahead.out to reflect the expanded probe patterns: change the source description from "blockdev --getra /dev/*" to something explicit like "`blockdev --getra` (sd/vd/xvd/nvme devices)" or "`blockdev --getra /dev/sd* /dev/vd* /dev/xvd* /dev/nvme*`" so it documents that the read_ahead collector now probes /dev/vd* (virtio) and /dev/xvd* (Xen) in addition to the original devices; locate the row by the symbol system/read_ahead.out and update its third-column description accordingly.
🧹 Nitpick comments (1)
DATA.md (1)
49-50: ⚖️ Poor tradeoff
DATA.mdanddocs/data.mdare identical — consider consolidating.Both files receive the same edits every time documentation changes. Maintaining two copies in lockstep is error-prone (a future update to one will silently diverge from the other). Consider making one a symlink or redirect to the other, or removing the duplicate.
Also applies to: 185-186, 227-227, 265-265, 267-267, 288-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DATA.md` around lines 49 - 50, The repository contains duplicate documentation files DATA.md and docs/data.md; consolidate them by keeping a single source of truth (choose either DATA.md or docs/data.md), remove the duplicate file, and replace it with a symlink or a redirect to the retained file so future edits only need to be made once; update any references or links in the repo and CI/docs build config to point to the retained file and verify examples like the `system/hostname.out`/`system/hostname_fqdn.out` entries still appear correctly in the remaining file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/data.md`:
- Around line 49-112: Update the table entry for system/read_ahead.out to
reflect the expanded probe patterns: change the source description from
"blockdev --getra /dev/*" to something explicit like "`blockdev --getra`
(sd/vd/xvd/nvme devices)" or "`blockdev --getra /dev/sd* /dev/vd* /dev/xvd*
/dev/nvme*`" so it documents that the read_ahead collector now probes /dev/vd*
(virtio) and /dev/xvd* (Xen) in addition to the original devices; locate the row
by the symbol system/read_ahead.out and update its third-column description
accordingly.
---
Nitpick comments:
In `@DATA.md`:
- Around line 49-50: The repository contains duplicate documentation files
DATA.md and docs/data.md; consolidate them by keeping a single source of truth
(choose either DATA.md or docs/data.md), remove the duplicate file, and replace
it with a symlink or a redirect to the retained file so future edits only need
to be made once; update any references or links in the repo and CI/docs build
config to point to the retained file and verify examples like the
`system/hostname.out`/`system/hostname_fqdn.out` entries still appear correctly
in the remaining file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3cd0013a-9091-40e0-a5c1-80a1a28610f6
📒 Files selected for processing (4)
DATA.mddocs/data.mdsystem_tasks_darwin.gosystem_tasks_linux.go
1 participant
Summary
Adds 5 new diagnostic collectors and extends 4 existing collectors with columns supporting deeper analysis: bloat, vacuum health, wraparound headroom, index validity/usage, sequence headroom, SSL audit, replication slot spill, extension version metadata. Also fixes the
read_aheadsystem collector and splits thehostnamecollector to disambiguate kernel hostname from resolver-canonical FQDN.New collectors:
stat_ssl(postgresql/) - per-backend SSL/TLS state frompg_stat_sslstat_replication_slots(postgresql/) - logical replication slot spill counters frompg_stat_replication_slots(PG14+)sequences(databases/{db}/) - sequences with last/min/max values frompg_sequencesbloat(databases/{db}/) - heuristic table bloat estimate frompg_statspgstattuple(databases/{db}/) - authoritative bloat viapgstattuple_approx()when the extension is installedExtended collectors:
databases-datfrozenxid,datminmxid,datconnlimit,datistemplate,datallowconntables- rebuilt onpg_class+pg_stat_all_tablesfor vacuum/analyze timestamps, dead-tup counters,reloptions,relpersistence, heap size (pg_relation_size), table size (pg_table_size),reltoastrelid/toast size; capped at top 1000 by sizeindexes- rebuilt onpg_index+pg_stat_all_indexesfor semantic-key columns,indisvalid,idx_scan/idx_tup_read/idx_tup_fetch, index size; capped at top 1000 by sizeavailable_extensions- now sourced frompg_available_extension_versionsfor per-versioninstalled/trusted/superuser/relocatable/requiresmetadataBug fixes:
read_aheadsystem collector now handles virtio (/dev/vd*) and xen (/dev/xvd*) block devices and gracefully skips unmatched globs; output is now labeled per device.hostnamesystem collector split:system/hostname.outnow contains plainhostname(kernel hostname /gethostname()), and a newsystem/hostname_fqdn.outcontainshostname -f(resolver canonical name).Checklist
TestQueryTaskColumnsCoverageasserts new column tokens andLIMIT 1000stay in the queries;TestPostgreSQLCollectorscoversstat_sslandstat_replication_slots)README.md,docs/index.md,DATA.md,docs/data.md)pgstattuplecorrectly skipped when extension absent;stat_replication_slotsPG14+ guarded by existing42P01SkipError path)oc_filecache, top-by-size ordering verified, pgstattuple correctly skipped across all 10 DBs)