diff --git a/internal/query/sizes_test.go b/internal/query/sizes_test.go index ce3d797..884f47b 100644 --- a/internal/query/sizes_test.go +++ b/internal/query/sizes_test.go @@ -30,3 +30,35 @@ func Test_StatSizesQueries(t *testing.T) { }) } } + +// Test_StatSizesQueries_NonDefaultSchema reproduces issue #116: the old query used +// (schemaname||'.'||relname)::regclass which failed for tables in non-default schemas +// when the name required quoting (mixed case, special chars) or the schema wasn't in +// search_path. The fix was to use s.relid (OID) instead. +func Test_StatSizesQueries_NonDefaultSchema(t *testing.T) { + conn, err := postgres.NewTestConnect() + if err != nil { + t.Skipf("postgres not available in test environment") + } + defer conn.Close() + + // Create a non-default schema and a table inside it. + _, err = conn.Exec(`CREATE SCHEMA IF NOT EXISTS test_dbo`) + assert.NoError(t, err) + + _, err = conn.Exec(`CREATE TABLE IF NOT EXISTS test_dbo.t1hlog (id int)`) + assert.NoError(t, err) + + defer func() { + _, _ = conn.Exec(`DROP TABLE IF EXISTS test_dbo.t1hlog`) + _, _ = conn.Exec(`DROP SCHEMA IF EXISTS test_dbo`) + }() + + opts := NewOptions(170000, "f", "off", 256, "public") + q, err := Format(PgTablesSizesDefault, opts) + assert.NoError(t, err) + + // Must not error with "relation does not exist" for tables in non-default schemas. + _, err = conn.Exec(q) + assert.NoError(t, err) +} 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 11c3760..4d9ff27 100644 --- a/internal/query/statements_test.go +++ b/internal/query/statements_test.go @@ -17,12 +17,17 @@ func TestSelectStatStatementsTimingQuery(t *testing.T) { {version: 100000, want: PgStatStatementsTimingPG12}, {version: 110000, want: PgStatStatementsTimingPG12}, {version: 120000, want: PgStatStatementsTimingPG12}, - {version: 130000, 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: PgStatStatementsTimingDefault}, + {version: 180000, want: PgStatStatementsTimingDefault}, } 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) { @@ -105,33 +112,42 @@ func TestSelectQueryReportQuery(t *testing.T) { {version: 100000, want: PgStatStatementsReportQueryPG12}, {version: 110000, want: PgStatStatementsReportQueryPG12}, {version: 120000, want: PgStatStatementsReportQueryPG12}, - {version: 130000, 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: PgStatStatementsReportQueryDefault}, + {version: 180000, want: PgStatStatementsReportQueryDefault}, } 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) + }) } } 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, 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}}, } diff --git a/top/stat.go b/top/stat.go index 6060ce4..59bde54 100644 --- a/top/stat.go +++ b/top/stat.go @@ -286,6 +286,22 @@ func formatInfoString(cfg postgres.Config, state, version, uptime, recovery stri ) } +// alignViewToResult ensures config.view.ColsWidth is consistent with r.Ncols. +// It is called before every render. The Aligned flag alone is insufficient because +// after a view switch the first stat batch may still carry the OLD view's column +// count: SetAlign then populates ColsWidth for the wrong number of columns, and the +// next batch (with the correct column count) skips realignment and reads zero widths +// for missing keys — causing "slice bounds out of range [:-1]" (issue #99). +func alignViewToResult(config *config, r stat.PGresult) { + if config.view.Aligned && len(config.view.ColsWidth) == r.Ncols { + return + } + widthes, cols := align.SetAlign(r, 1000, false) // high limit avoids truncating the last value + config.view.Cols = cols + config.view.ColsWidth = widthes + config.view.Aligned = true +} + // printDbstat prints main Postgres stats on UI. func printDbstat(v *gocui.View, config *config, s stat.Stat) error { // If reading stats failed, print the error occurred and return. @@ -299,12 +315,7 @@ func printDbstat(v *gocui.View, config *config, s stat.Stat) error { } // Align values within columns, use fixed aligning instead of dynamic. - if !config.view.Aligned { - widthes, cols := align.SetAlign(s.Result, 1000, false) // use high limit (1000) to avoid truncating last value. - config.view.Cols = cols - config.view.ColsWidth = widthes - config.view.Aligned = true - } + alignViewToResult(config, s.Result) // Print header. err := printStatHeader(v, s, config) diff --git a/top/stat_test.go b/top/stat_test.go index 5c6b1d3..defc355 100644 --- a/top/stat_test.go +++ b/top/stat_test.go @@ -1,10 +1,13 @@ package top import ( + "database/sql" "fmt" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgconn" "github.com/lesovsky/pgcenter/internal/postgres" + "github.com/lesovsky/pgcenter/internal/stat" + "github.com/lesovsky/pgcenter/internal/view" "github.com/stretchr/testify/assert" "testing" ) @@ -47,3 +50,63 @@ func Test_formatError(t *testing.T) { assert.Equal(t, tc.want, got) } } + +// Test_alignViewToResult reproduces issue #99: pressing 'x' to cycle pg_stat_statements +// views caused "slice bounds out of range [:-1]" / "zero or negative width, skip". +// +// Root cause: after a view switch, the first stat batch may still carry the OLD view's +// column count. SetAlign fires on it (Aligned=false), populating ColsWidth for N columns. +// The next batch has M > N columns from the new view, but Aligned=true skips SetAlign, +// so ColsWidth[N..M-1] returns 0 → panic or error on truncation. +// +// Fix: alignViewToResult also re-aligns when len(ColsWidth) != r.Ncols. +func Test_alignViewToResult(t *testing.T) { + makeResult := func(ncols int) stat.PGresult { + cols := make([]string, ncols) + row := make([]sql.NullString, ncols) + for i := 0; i < ncols; i++ { + cols[i] = fmt.Sprintf("col%d", i+1) + row[i] = sql.NullString{String: "value", Valid: true} + } + return stat.PGresult{Valid: true, Ncols: ncols, Nrows: 1, Cols: cols, + Values: [][]sql.NullString{row}} + } + + t.Run("first render sets alignment", func(t *testing.T) { + cfg := &config{view: view.View{Aligned: false, ColsWidth: map[int]int{}}} + alignViewToResult(cfg, makeResult(8)) + assert.True(t, cfg.view.Aligned) + assert.Equal(t, 8, len(cfg.view.ColsWidth)) + }) + + t.Run("no re-alignment when column count matches", func(t *testing.T) { + original := map[int]int{0: 99, 1: 99, 2: 99} + cfg := &config{view: view.View{Aligned: true, ColsWidth: original}} + alignViewToResult(cfg, makeResult(3)) + assert.Equal(t, 99, cfg.view.ColsWidth[0], "ColsWidth must not change when counts match") + }) + + t.Run("re-aligns when new result has MORE columns than ColsWidth (was: panic)", func(t *testing.T) { + // Simulate: view was aligned with 5 columns (e.g. statements_general), + // then a batch with 13 columns arrives (e.g. statements_timings after rapid 'x'). + // Before fix: ColsWidth[5..12] = 0 → "slice bounds out of range [:-1]". + cfg := &config{view: view.View{ + Aligned: true, + ColsWidth: map[int]int{0: 10, 1: 10, 2: 10, 3: 10, 4: 10}, + }} + alignViewToResult(cfg, makeResult(13)) + assert.Equal(t, 13, len(cfg.view.ColsWidth)) + for i := 0; i < 13; i++ { + assert.Greater(t, cfg.view.ColsWidth[i], 0, "ColsWidth[%d] must be > 0", i) + } + }) + + t.Run("re-aligns when new result has FEWER columns than ColsWidth", func(t *testing.T) { + cfg := &config{view: view.View{ + Aligned: true, + ColsWidth: map[int]int{0: 10, 1: 10, 2: 10, 3: 10, 4: 10, 5: 10, 6: 10}, + }} + alignViewToResult(cfg, makeResult(4)) + assert.Equal(t, 4, len(cfg.view.ColsWidth)) + }) +}