From dbcfac147befd3721971314e350ae8ddedbef1ed Mon Sep 17 00:00:00 2001 From: Alexey Lesovsky Date: Mon, 18 May 2026 18:10:51 +0500 Subject: [PATCH 1/3] Fix test coverage gaps for pg_stat_statements on PG 17+ (issue #127) Two structural problems in statements tests: 1. t.Skipf inside a bare for-loop caused the entire timing/report/wal suite to be skipped when the first PG version (9.5) was unavailable. Timing and report query integration tests never ran against PG 17/18. 2. TestSelectStatStatementsTimingQuery and TestSelectQueryReportQuery only asserted version routing up to PG 13, so a regression in the PG 17+ routing would go undetected. Fixes: - Split timing and WAL loops into per-version t.Run sub-tests; missing instances now skip only that version, not the entire suite. - Extend WAL stats test to cover PG 13-18 (was hardcoded to PG 13). - Add PG 14/16/17/18 cases to both unit routing tests, asserting that PG 17+ returns the blk_read_time-free PG17 query variants. Co-Authored-By: Claude Sonnet 4.6 --- internal/query/statements_test.go | 74 +++++++++++++++++++------------ 1 file changed, 45 insertions(+), 29 deletions(-) diff --git a/internal/query/statements_test.go b/internal/query/statements_test.go index 11c3760..c9736d3 100644 --- a/internal/query/statements_test.go +++ b/internal/query/statements_test.go @@ -18,11 +18,16 @@ func TestSelectStatStatementsTimingQuery(t *testing.T) { {version: 110000, want: PgStatStatementsTimingPG12}, {version: 120000, want: PgStatStatementsTimingPG12}, {version: 130000, want: PgStatStatementsTimingDefault}, + {version: 140000, want: PgStatStatementsTimingDefault}, + {version: 160000, want: PgStatStatementsTimingDefault}, + // PG 17+: blk_read_time/blk_write_time replaced by shared_blk_read_time etc. + {version: 170000, want: PgStatStatementsTimingPG17}, + {version: 180000, want: PgStatStatementsTimingPG17}, } for _, tc := range testcases { got := SelectStatStatementsTimingQuery(tc.version) - assert.Equal(t, tc.want, got) + assert.Equal(t, tc.want, got, "version %d", tc.version) } } @@ -56,8 +61,11 @@ func Test_StatStatementsQueries(t *testing.T) { }) } - t.Run("pg_stat_statements_timing", func(t *testing.T) { - for _, version := range versions { + // Each version in its own sub-test so a missing PG instance skips only that version, + // not the entire timing suite (fixes the t.Skipf-in-loop bug). + for _, version := range versions { + version := version + t.Run(fmt.Sprintf("pg_stat_statements_timing/%d", version), func(t *testing.T) { tmpl := SelectStatStatementsTimingQuery(version) opts := NewOptions(version, "f", "off", 256, "public") q, err := Format(tmpl, opts) @@ -67,32 +75,31 @@ func Test_StatStatementsQueries(t *testing.T) { if err != nil { t.Skipf("postgres %d not available in test environment", version) } + defer conn.Close() _, err = conn.Exec(q) assert.NoError(t, err) + }) + } - conn.Close() - } - }) - - t.Run("pg_stat_statements_wal", func(t *testing.T) { - for _, version := range []int{130000} { - tmpl := PgStatStatementsWalDefault + // WAL stats are available since PG 13; test all supported versions. + for _, version := range []int{130000, 140000, 150000, 160000, 170000, 180000} { + version := version + t.Run(fmt.Sprintf("pg_stat_statements_wal/%d", version), func(t *testing.T) { opts := NewOptions(version, "f", "off", 256, "public") - q, err := Format(tmpl, opts) + q, err := Format(PgStatStatementsWalDefault, opts) assert.NoError(t, err) conn, err := postgres.NewTestConnectVersion(version) if err != nil { t.Skipf("postgres %d not available in test environment", version) } + defer conn.Close() _, err = conn.Exec(q) assert.NoError(t, err) - - conn.Close() - } - }) + }) + } } func TestSelectQueryReportQuery(t *testing.T) { @@ -106,32 +113,41 @@ func TestSelectQueryReportQuery(t *testing.T) { {version: 110000, want: PgStatStatementsReportQueryPG12}, {version: 120000, want: PgStatStatementsReportQueryPG12}, {version: 130000, want: PgStatStatementsReportQueryDefault}, + {version: 140000, want: PgStatStatementsReportQueryDefault}, + {version: 160000, want: PgStatStatementsReportQueryDefault}, + // PG 17+: blk_read_time/blk_write_time replaced by shared_blk_read_time etc. + {version: 170000, want: PgStatStatementsReportQueryPG17}, + {version: 180000, want: PgStatStatementsReportQueryPG17}, } for _, tc := range testcases { got := SelectQueryReportQuery(tc.version) - assert.Equal(t, tc.want, got) + assert.Equal(t, tc.want, got, "version %d", tc.version) } } +// Test_StatStatementsReportQueries runs each version in its own sub-test so a missing +// PG instance skips only that version, not the entire suite (fixes the t.Skipf-in-loop bug). func Test_StatStatementsReportQueries(t *testing.T) { versions := []int{90500, 90600, 100000, 110000, 120000, 130000, 140000, 150000, 160000, 170000, 180000} for _, version := range versions { - tmpl := SelectQueryReportQuery(version) - opts := NewOptions(version, "f", "off", 256, "public") - q, err := Format(tmpl, opts) - assert.NoError(t, err) - - conn, err := postgres.NewTestConnectVersion(version) - if err != nil { - t.Skipf("postgres %d not available in test environment", version) - } + version := version + t.Run(fmt.Sprintf("version/%d", version), func(t *testing.T) { + tmpl := SelectQueryReportQuery(version) + opts := NewOptions(version, "f", "off", 256, "public") + q, err := Format(tmpl, opts) + assert.NoError(t, err) - // Use fake query_id, just test queries are executed with no errors. - _, err = conn.Exec(q, "1234567890") - assert.NoError(t, err) + conn, err := postgres.NewTestConnectVersion(version) + if err != nil { + t.Skipf("postgres %d not available in test environment", version) + } + defer conn.Close() - conn.Close() + // Use fake query_id, just test queries are executed with no errors. + _, err = conn.Exec(q, "1234567890") + assert.NoError(t, err) + }) } } From 1a98105d2c695ac2f0d6bbf787c0ba471ee4aa89 Mon Sep 17 00:00:00 2001 From: Alexey Lesovsky Date: Mon, 18 May 2026 18:17:02 +0500 Subject: [PATCH 2/3] Align versioned query naming: Default always refers to the latest PG version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Convention: PGxx suffix marks a query valid from PG xx (now superseded); Default marks the query for the current/latest supported version. Previously violated by two files: - wal.go: PgStatWALDefault covered PG 14-17 (not latest); PgStatWALPG18 covered PG 18+ (latest). - statements.go: PgStatStatementsTimingDefault / ReportQueryDefault covered PG 13-16; PG17 variants covered PG 17+ (latest). Renames: - PgStatWALDefault → PgStatWALPG14 - PgStatWALPG18 → PgStatWALDefault - PgStatStatementsTimingDefault → PgStatStatementsTimingPG13 - PgStatStatementsTimingPG17 → PgStatStatementsTimingDefault - PgStatStatementsReportQueryDefault → PgStatStatementsReportQueryPG13 - PgStatStatementsReportQueryPG17 → PgStatStatementsReportQueryDefault Updated: selector functions, view.go initial templates, all test references. Co-Authored-By: Claude Sonnet 4.6 --- internal/query/statements.go | 26 ++++++++++++++------------ internal/query/statements_test.go | 20 ++++++++++---------- internal/query/wal.go | 12 ++++++------ internal/view/view.go | 4 ++-- 4 files changed, 32 insertions(+), 30 deletions(-) diff --git a/internal/query/statements.go b/internal/query/statements.go index 29aa409..29e89e2 100644 --- a/internal/query/statements.go +++ b/internal/query/statements.go @@ -4,8 +4,8 @@ const ( // NOTES: // 1. regexp_replace() removes extra spaces, tabs and newlines from queries - // PgStatStatementsTimingDefault defines default query for getting timings stats from pg_stat_statements view. - PgStatStatementsTimingDefault = "SELECT pg_get_userbyid(p.userid) AS user, d.datname AS database, " + + // PgStatStatementsTimingPG13 defines timing query for pg_stat_statements (PG 13-16). + PgStatStatementsTimingPG13 = "SELECT pg_get_userbyid(p.userid) AS user, d.datname AS database, " + "date_trunc('seconds', round(p.total_plan_time + p.total_exec_time) / 1000 * '1 second'::interval)::text AS all_total, " + "date_trunc('seconds', round(p.blk_read_time) / 1000 * '1 second'::interval)::text AS read_total, " + "date_trunc('seconds', round(p.blk_write_time) / 1000 * '1 second'::interval)::text AS write_total, " + @@ -29,8 +29,9 @@ const ( `regexp_replace({{.PgSSQueryLenFn}}, E'\\s+', ' ', 'g') AS query ` + "FROM {{.PGSSSchema}}.pg_stat_statements p JOIN pg_database d ON d.oid=p.dbid" - // the timing query for Postgres 17 and newer. v17 splits up the read-write timing stats into more granular columns. - PgStatStatementsTimingPG17 = ` + // PgStatStatementsTimingDefault defines timing query for pg_stat_statements (PG 17+). + // PG 17 splits read/write timing into shared_blk_*, local_blk_*, temp_blk_* columns. + PgStatStatementsTimingDefault = ` SELECT pg_get_userbyid(p.userid) AS user, d.datname AS database, date_trunc('seconds', round(p.total_exec_time + p.total_plan_time) / 1000 * '1 second'::interval)::text AS all_total, date_trunc('seconds', round(p.shared_blk_read_time + p.local_blk_read_time + p.temp_blk_read_time) / 1000 * '1 second'::interval)::text AS read_total, @@ -96,8 +97,8 @@ FROM {{.PGSSSchema}}.pg_stat_statements p JOIN pg_database d ON d.oid=p.dbid `regexp_replace({{.PgSSQueryLenFn}}, E'\\s+', ' ', 'g') AS query ` + "FROM {{.PGSSSchema}}.pg_stat_statements p JOIN pg_database d ON d.oid=p.dbid" - // PgStatStatementsReportQueryDefault defines query used for calculating per-statement report based on pg_stat_statements. - PgStatStatementsReportQueryDefault = "WITH totals AS (SELECT " + + // PgStatStatementsReportQueryPG13 defines report query for pg_stat_statements (PG 13-16). + PgStatStatementsReportQueryPG13 = "WITH totals AS (SELECT " + "sum(calls) AS total_calls," + "sum(rows) AS total_rows," + "sum(total_plan_time + total_exec_time) AS total_all_time," + @@ -158,8 +159,9 @@ FROM {{.PGSSSchema}}.pg_stat_statements p JOIN pg_database d ON d.oid=p.dbid "to_char(100*s.wal_bytes/(SELECT coalesce(nullif(total_wal_bytes, 0), 1) FROM totals), 'FM990.00') AS wal_bytes_ratio " + "FROM stmt s JOIN pg_database d ON d.oid=s.dbid LIMIT 1" - // The reports query for Postgres 17 and newer. v17 splits up the read-write timing stats into more granular columns. - PgStatStatementsReportQueryPG17 = ` + // PgStatStatementsReportQueryDefault defines report query for pg_stat_statements (PG 17+). + // PG 17 splits read/write timing into shared_blk_*, local_blk_*, temp_blk_* columns. + PgStatStatementsReportQueryDefault = ` WITH totals AS ( SELECT sum(calls) AS total_calls, @@ -306,9 +308,9 @@ func SelectStatStatementsTimingQuery(version int) string { case version < 130000: return PgStatStatementsTimingPG12 case version >= 170000: - return PgStatStatementsTimingPG17 - default: return PgStatStatementsTimingDefault + default: + return PgStatStatementsTimingPG13 } } @@ -318,8 +320,8 @@ func SelectQueryReportQuery(version int) string { case version < 130000: return PgStatStatementsReportQueryPG12 case version >= 170000: - return PgStatStatementsReportQueryPG17 - default: return PgStatStatementsReportQueryDefault + default: + return PgStatStatementsReportQueryPG13 } } diff --git a/internal/query/statements_test.go b/internal/query/statements_test.go index c9736d3..4d9ff27 100644 --- a/internal/query/statements_test.go +++ b/internal/query/statements_test.go @@ -17,12 +17,12 @@ func TestSelectStatStatementsTimingQuery(t *testing.T) { {version: 100000, want: PgStatStatementsTimingPG12}, {version: 110000, want: PgStatStatementsTimingPG12}, {version: 120000, want: PgStatStatementsTimingPG12}, - {version: 130000, want: PgStatStatementsTimingDefault}, - {version: 140000, want: PgStatStatementsTimingDefault}, - {version: 160000, want: PgStatStatementsTimingDefault}, + {version: 130000, want: PgStatStatementsTimingPG13}, + {version: 140000, want: PgStatStatementsTimingPG13}, + {version: 160000, want: PgStatStatementsTimingPG13}, // PG 17+: blk_read_time/blk_write_time replaced by shared_blk_read_time etc. - {version: 170000, want: PgStatStatementsTimingPG17}, - {version: 180000, want: PgStatStatementsTimingPG17}, + {version: 170000, want: PgStatStatementsTimingDefault}, + {version: 180000, want: PgStatStatementsTimingDefault}, } for _, tc := range testcases { @@ -112,12 +112,12 @@ func TestSelectQueryReportQuery(t *testing.T) { {version: 100000, want: PgStatStatementsReportQueryPG12}, {version: 110000, want: PgStatStatementsReportQueryPG12}, {version: 120000, want: PgStatStatementsReportQueryPG12}, - {version: 130000, want: PgStatStatementsReportQueryDefault}, - {version: 140000, want: PgStatStatementsReportQueryDefault}, - {version: 160000, want: PgStatStatementsReportQueryDefault}, + {version: 130000, want: PgStatStatementsReportQueryPG13}, + {version: 140000, want: PgStatStatementsReportQueryPG13}, + {version: 160000, want: PgStatStatementsReportQueryPG13}, // PG 17+: blk_read_time/blk_write_time replaced by shared_blk_read_time etc. - {version: 170000, want: PgStatStatementsReportQueryPG17}, - {version: 180000, want: PgStatStatementsReportQueryPG17}, + {version: 170000, want: PgStatStatementsReportQueryDefault}, + {version: 180000, want: PgStatStatementsReportQueryDefault}, } for _, tc := range testcases { diff --git a/internal/query/wal.go b/internal/query/wal.go index e1862d6..c17286d 100644 --- a/internal/query/wal.go +++ b/internal/query/wal.go @@ -1,8 +1,8 @@ package query const ( - // PgStatWALDefault defines query for pg_stat_wal (PG 14-17). - PgStatWALDefault = "SELECT 'WAL' AS source, " + + // PgStatWALPG14 defines query for pg_stat_wal (PG 14-17). + PgStatWALPG14 = "SELECT 'WAL' AS source, " + "(SELECT pg_size_pretty(count(1) * pg_size_bytes(current_setting('wal_segment_size'))) AS waldir_size FROM pg_ls_waldir()) AS waldir_size, " + `round(wal_bytes / 1024, 2) AS "wal,KiB", ` + "wal_records AS records, wal_fpi AS fpi, " + @@ -10,9 +10,9 @@ const ( "date_trunc('seconds', now() - stats_reset)::text AS stats_age " + "FROM pg_stat_wal" - // PgStatWALPG18 defines query for pg_stat_wal (PG 18+). + // PgStatWALDefault defines query for pg_stat_wal (PG 18+). // wal_write, wal_sync, wal_write_time, wal_sync_time removed in PG 18. - PgStatWALPG18 = "SELECT 'WAL' AS source, " + + PgStatWALDefault = "SELECT 'WAL' AS source, " + "(SELECT pg_size_pretty(count(1) * pg_size_bytes(current_setting('wal_segment_size'))) AS waldir_size FROM pg_ls_waldir()) AS waldir_size, " + `round(wal_bytes / 1024, 2) AS "wal,KiB", ` + "wal_records AS records, wal_fpi AS fpi, " + @@ -25,8 +25,8 @@ const ( func SelectStatWALQuery(version int) (string, int, [2]int) { if version >= 180000 { // PG 18 removed wal_write/wal_sync columns; stats_age is col 6 and must not be diffed. - return PgStatWALPG18, 7, [2]int{2, 5} + return PgStatWALDefault, 7, [2]int{2, 5} } // cols 2-9: wal,KiB..buffers_full; col 10 (stats_age) excluded. - return PgStatWALDefault, 11, [2]int{2, 9} + return PgStatWALPG14, 11, [2]int{2, 9} } diff --git a/internal/view/view.go b/internal/view/view.go index 4d8a13d..a8f8b60 100644 --- a/internal/view/view.go +++ b/internal/view/view.go @@ -124,7 +124,7 @@ func New() Views { "wal": { Name: "wal", MinRequiredVersion: query.PostgresV14, - QueryTmpl: query.PgStatWALDefault, + QueryTmpl: query.PgStatWALPG14, DiffIntvl: [2]int{2, 9}, Ncols: 11, OrderKey: 0, @@ -135,7 +135,7 @@ func New() Views { }, "statements_timings": { Name: "statements_timings", - QueryTmpl: query.PgStatStatementsTimingDefault, + QueryTmpl: query.PgStatStatementsTimingPG13, DiffIntvl: [2]int{6, 10}, Ncols: 13, OrderKey: 0, From 272fd05f5a4a3642af09cea15b88b239cfcbb09a Mon Sep 17 00:00:00 2001 From: Alexey Lesovsky Date: Mon, 18 May 2026 18:35:34 +0500 Subject: [PATCH 3/3] Fix 'invalid result' when reading tar files recorded after cbfa0a4 (issue #122) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit cbfa0a4 added shared_preload_libraries to SelectCommonProperties, increasing the metadata column count from 7 to 8. readMeta's strict "Ncols != 7" check rejected all tar files recorded after that commit, printing "invalid result" for every report row. Fix: replace "!= 7" with "< 2" — readMeta only reads column 1 (version_num), so the minimum required is 2 columns. This accepts both old (7-col) and current (8-col) metadata without breaking forward compatibility for any future column additions. Adds a regression test with 8-column metadata that previously failed. Co-Authored-By: Claude Sonnet 4.6 --- report/report.go | 5 ++++- report/report_test.go | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/report/report.go b/report/report.go index c7262ec..89bc2ce 100644 --- a/report/report.go +++ b/report/report.go @@ -289,8 +289,11 @@ func processData(app *app, v view.View, config Config, dataCh chan data, doneCh } // readMeta creates metadata object from stat.PGresult. +// The metadata query SelectCommonProperties had 7 columns before cbfa0a4 (without +// shared_preload_libraries) and 8 columns after. Accept any result with at least 2 +// columns so that tar files recorded with either version can be read. func readMeta(res stat.PGresult) (metadata, error) { - if res.Nrows != 1 || res.Ncols != 7 { + if res.Nrows != 1 || res.Ncols < 2 { return metadata{}, fmt.Errorf("invalid result") } diff --git a/report/report_test.go b/report/report_test.go index 1dcb16f..2accadf 100644 --- a/report/report_test.go +++ b/report/report_test.go @@ -338,6 +338,25 @@ func Test_readMeta(t *testing.T) { }, want: metadata{version: 140000}, }, + // Reproduces issue #122: shared_preload_libraries was added to SelectCommonProperties + // in cbfa0a4, making it 8 columns. Tar files recorded after that commit have Ncols=8 + // and were incorrectly rejected by the strict "!= 7" check. + { + valid: true, + res: stat.PGresult{ + Values: [][]sql.NullString{ + { + {String: "14.9", Valid: true}, {String: "140009", Valid: true}, + {String: "off", Valid: true}, {String: "100", Valid: true}, {String: "3", Valid: true}, + {String: "pg_stat_statements", Valid: true}, + {String: "false", Valid: true}, {String: "1622828486655396e-6", Valid: true}, + }, + }, + Cols: []string{"version", "version_num", "track_commit_timestamp", "max_connections", "autovacuum_max_workers", "shared_preload_libraries", "recovery", "start_time_unix"}, + Ncols: 8, Nrows: 1, Valid: true, + }, + want: metadata{version: 140009}, + }, {valid: false, res: stat.PGresult{Ncols: 1, Nrows: 1, Valid: true}}, {valid: false, res: stat.PGresult{Ncols: 7, Nrows: 0, Valid: true}}, }