From 1b07d8d03400e624f5be383908624eaef4b5b59c Mon Sep 17 00:00:00 2001 From: chieflibrarian <323671055+chieflibrarian@users.noreply.github.com> Date: Thu, 1 Oct 2026 16:22:46 +0200 Subject: [PATCH] fix: add missing rows.Err() check in pg_stat_database.go (#1399) Ensure any row preparation errors are validated by returning rows.Err() after the rows.Next() loop finishes in collector/pg_stat_database. Added a unit test and refactored the way functions return rows.Err() within the codebase for consistency. Signed-off-by: chief librarian <323671055+chieflibrarian@users.noreply.github.com> --- collector/pg_database.go | 1 + collector/pg_database_wraparound.go | 6 +- collector/pg_locks.go | 6 +- collector/pg_long_running_transactions.go | 6 +- collector/pg_replication_slots.go | 1 + collector/pg_stat_activity_autovacuum.go | 6 +- collector/pg_stat_database.go | 7 +- collector/pg_stat_database_test.go | 165 ++++++++++------------ collector/pg_stat_progress_vacuum.go | 5 +- collector/pg_stat_statements.go | 6 +- collector/pg_stat_user_tables.go | 5 +- collector/pg_stat_walreceiver.go | 6 +- collector/pg_statio_user_indexes.go | 6 +- collector/pg_statio_user_tables.go | 1 + collector/pg_xlog_location.go | 6 +- 15 files changed, 102 insertions(+), 131 deletions(-) diff --git a/collector/pg_database.go b/collector/pg_database.go index 3e27570f..fc68c17a 100644 --- a/collector/pg_database.go +++ b/collector/pg_database.go @@ -135,5 +135,6 @@ func (c PGDatabaseCollector) Update(ctx context.Context, instance *instance, ch ) } + return rows.Err() } diff --git a/collector/pg_database_wraparound.go b/collector/pg_database_wraparound.go index 776787ec..47fab4cf 100644 --- a/collector/pg_database_wraparound.go +++ b/collector/pg_database_wraparound.go @@ -105,8 +105,6 @@ func (c *PGDatabaseWraparoundCollector) Update(ctx context.Context, instance *in ageDatminmxidMetric, datname.String, ) } - if err := rows.Err(); err != nil { - return err - } - return nil + + return rows.Err() } diff --git a/collector/pg_locks.go b/collector/pg_locks.go index 8515de96..f80f70a7 100644 --- a/collector/pg_locks.go +++ b/collector/pg_locks.go @@ -120,8 +120,6 @@ func (c PGLocksCollector) Update(ctx context.Context, instance *instance, ch cha datname.String, mode.String, ) } - if err := rows.Err(); err != nil { - return err - } - return nil + + return rows.Err() } diff --git a/collector/pg_long_running_transactions.go b/collector/pg_long_running_transactions.go index 1ab238c6..cdf3241e 100644 --- a/collector/pg_long_running_transactions.go +++ b/collector/pg_long_running_transactions.go @@ -110,8 +110,6 @@ func (c PGLongRunningTransactionsCollector) Update(ctx context.Context, instance age, ) } - if err := rows.Err(); err != nil { - return err - } - return nil + + return rows.Err() } diff --git a/collector/pg_replication_slots.go b/collector/pg_replication_slots.go index 1d79a427..06df7556 100644 --- a/collector/pg_replication_slots.go +++ b/collector/pg_replication_slots.go @@ -280,6 +280,7 @@ func updateReplicationSlotsSlotMetrics(ctx context.Context, instance *instance, ) } } + return rows.Err() } diff --git a/collector/pg_stat_activity_autovacuum.go b/collector/pg_stat_activity_autovacuum.go index 10350b5b..67864d68 100644 --- a/collector/pg_stat_activity_autovacuum.go +++ b/collector/pg_stat_activity_autovacuum.go @@ -81,8 +81,6 @@ func (c PGStatActivityAutovacuumCollector) Update(ctx context.Context, instance ageInSeconds.Float64, relname, ) } - if err := rows.Err(); err != nil { - return err - } - return nil + + return rows.Err() } diff --git a/collector/pg_stat_database.go b/collector/pg_stat_database.go index 5e29b319..37d46068 100644 --- a/collector/pg_stat_database.go +++ b/collector/pg_stat_database.go @@ -251,9 +251,7 @@ func (c *PGStatDatabaseCollector) Update(ctx context.Context, instance *instance columns = append(columns, "active_time") } - rows, err := db.QueryContext(ctx, - statDatabaseQuery(columns), - ) + rows, err := db.QueryContext(ctx, statDatabaseQuery(columns)) if err != nil { return err } @@ -511,5 +509,6 @@ func (c *PGStatDatabaseCollector) Update(ctx context.Context, instance *instance ) } } - return nil + + return rows.Err() } diff --git a/collector/pg_stat_database_test.go b/collector/pg_stat_database_test.go index e6194ca2..63b37507 100644 --- a/collector/pg_stat_database_test.go +++ b/collector/pg_stat_database_test.go @@ -14,6 +14,7 @@ package collector import ( "context" + "errors" "testing" "time" @@ -25,16 +26,8 @@ import ( "github.com/smartystreets/goconvey/convey" ) -func TestPGStatDatabaseCollector(t *testing.T) { - db, mock, err := sqlmock.New() - if err != nil { - t.Fatalf("Error opening a stub db connection: %s", err) - } - defer db.Close() - - inst := &instance{db: db, version: semver.MustParse("14.0.0")} - - columns := []string{ +var ( + pgStatCollectorTestColumns = []string{ "datid", "datname", "numbackends", @@ -56,13 +49,23 @@ func TestPGStatDatabaseCollector(t *testing.T) { "stats_reset", "active_time", } +) + +func TestPGStatDatabaseCollector(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("Error opening a stub db connection: %s", err) + } + defer db.Close() + + inst := &instance{db: db, version: semver.MustParse("14.0.0")} srT, err := time.Parse("2006-01-02 15:04:05.00000-07", "2023-05-25 17:10:42.81132-07") if err != nil { t.Fatalf("Error parsing time: %s", err) } - rows := sqlmock.NewRows(columns). + rows := sqlmock.NewRows(pgStatCollectorTestColumns). AddRow( "pid", "postgres", @@ -86,7 +89,7 @@ func TestPGStatDatabaseCollector(t *testing.T) { 33, ) - mock.ExpectQuery(sanitizeQuery(statDatabaseQuery(columns))).WillReturnRows(rows) + mock.ExpectQuery(sanitizeQuery(statDatabaseQuery(pgStatCollectorTestColumns))).WillReturnRows(rows) ch := make(chan prometheus.Metric) go func() { @@ -145,30 +148,7 @@ func TestPGStatDatabaseCollectorNullValues(t *testing.T) { } inst := &instance{db: db, version: semver.MustParse("14.0.0")} - columns := []string{ - "datid", - "datname", - "numbackends", - "xact_commit", - "xact_rollback", - "blks_read", - "blks_hit", - "tup_returned", - "tup_fetched", - "tup_inserted", - "tup_updated", - "tup_deleted", - "conflicts", - "temp_files", - "temp_bytes", - "deadlocks", - "blk_read_time", - "blk_write_time", - "stats_reset", - "active_time", - } - - rows := sqlmock.NewRows(columns). + rows := sqlmock.NewRows(pgStatCollectorTestColumns). AddRow( nil, "postgres", @@ -213,7 +193,7 @@ func TestPGStatDatabaseCollectorNullValues(t *testing.T) { srT, 32, ) - mock.ExpectQuery(sanitizeQuery(statDatabaseQuery(columns))).WillReturnRows(rows) + mock.ExpectQuery(sanitizeQuery(statDatabaseQuery(pgStatCollectorTestColumns))).WillReturnRows(rows) ch := make(chan prometheus.Metric) go func() { @@ -267,35 +247,12 @@ func TestPGStatDatabaseCollectorRowLeakTest(t *testing.T) { inst := &instance{db: db, version: semver.MustParse("14.0.0")} - columns := []string{ - "datid", - "datname", - "numbackends", - "xact_commit", - "xact_rollback", - "blks_read", - "blks_hit", - "tup_returned", - "tup_fetched", - "tup_inserted", - "tup_updated", - "tup_deleted", - "conflicts", - "temp_files", - "temp_bytes", - "deadlocks", - "blk_read_time", - "blk_write_time", - "stats_reset", - "active_time", - } - srT, err := time.Parse("2006-01-02 15:04:05.00000-07", "2023-05-25 17:10:42.81132-07") if err != nil { t.Fatalf("Error parsing time: %s", err) } - rows := sqlmock.NewRows(columns). + rows := sqlmock.NewRows(pgStatCollectorTestColumns). AddRow( "pid", "postgres", @@ -362,7 +319,7 @@ func TestPGStatDatabaseCollectorRowLeakTest(t *testing.T) { srT, 15, ) - mock.ExpectQuery(sanitizeQuery(statDatabaseQuery(columns))).WillReturnRows(rows) + mock.ExpectQuery(sanitizeQuery(statDatabaseQuery(pgStatCollectorTestColumns))).WillReturnRows(rows) ch := make(chan prometheus.Metric) go func() { @@ -436,30 +393,7 @@ func TestPGStatDatabaseCollectorTestNilStatReset(t *testing.T) { inst := &instance{db: db, version: semver.MustParse("14.0.0")} - columns := []string{ - "datid", - "datname", - "numbackends", - "xact_commit", - "xact_rollback", - "blks_read", - "blks_hit", - "tup_returned", - "tup_fetched", - "tup_inserted", - "tup_updated", - "tup_deleted", - "conflicts", - "temp_files", - "temp_bytes", - "deadlocks", - "blk_read_time", - "blk_write_time", - "stats_reset", - "active_time", - } - - rows := sqlmock.NewRows(columns). + rows := sqlmock.NewRows(pgStatCollectorTestColumns). AddRow( "pid", "postgres", @@ -483,7 +417,7 @@ func TestPGStatDatabaseCollectorTestNilStatReset(t *testing.T) { 7, ) - mock.ExpectQuery(sanitizeQuery(statDatabaseQuery(columns))).WillReturnRows(rows) + mock.ExpectQuery(sanitizeQuery(statDatabaseQuery(pgStatCollectorTestColumns))).WillReturnRows(rows) ch := make(chan prometheus.Metric) go func() { @@ -528,3 +462,60 @@ func TestPGStatDatabaseCollectorTestNilStatReset(t *testing.T) { t.Errorf("there were unfulfilled exceptions: %s", err) } } + +func TestPGStatDatabaseCollectorRowsError(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("Error opening a stub db connection: %s", err) + } + defer db.Close() + + inst := &instance{db: db, version: semver.MustParse("14.0.0")} + + expectedRowErr := errors.New("test row error") + rows := sqlmock.NewRows(pgStatCollectorTestColumns). + AddRow( + "pid", + "postgres", + 354, + 4945, + 289097744, + 1242257, + int64(3275602074), + 89320867, + 450139, + 2034563757, + 0, + int64(2725688749), + 23, + 52, + 74, + 925, + 16, + 823, + nil, + 7, + ).RowError(0, expectedRowErr) + + mock.ExpectQuery(sanitizeQuery(statDatabaseQuery(pgStatCollectorTestColumns))).WillReturnRows(rows) + + convey.Convey("With a buffered channel and valid collector", t, func() { + ch := make(chan prometheus.Metric, 1) + defer close(ch) + + c := PGStatDatabaseCollector{ + log: promslog.NewNopLogger().With("collector", "pg_stat_database"), + } + + convey.Convey("When doing an Update", func() { + err := c.Update(context.Background(), inst, ch) + + convey.Convey("It should return expected row error, with the channel remaining empty", func() { + convey.So(err, convey.ShouldBeError, expectedRowErr) + convey.So(ch, convey.ShouldBeEmpty) + + convey.So(mock.ExpectationsWereMet(), convey.ShouldBeNil) + }) + }) + }) +} diff --git a/collector/pg_stat_progress_vacuum.go b/collector/pg_stat_progress_vacuum.go index bd1fd673..b74602e3 100644 --- a/collector/pg_stat_progress_vacuum.go +++ b/collector/pg_stat_progress_vacuum.go @@ -220,8 +220,5 @@ func (c *PGStatProgressVacuumCollector) Update(ctx context.Context, instance *in ch <- prometheus.MustNewConstMetric(statProgressVacuumNumDeadTuples, prometheus.GaugeValue, numDead, labels...) } - if err := rows.Err(); err != nil { - return err - } - return nil + return rows.Err() } diff --git a/collector/pg_stat_statements.go b/collector/pg_stat_statements.go index 34970c93..eb76c780 100644 --- a/collector/pg_stat_statements.go +++ b/collector/pg_stat_statements.go @@ -326,10 +326,8 @@ func (c PGStatStatementsCollector) Update(ctx context.Context, instance *instanc } } } - if err := rows.Err(); err != nil { - return err - } - return nil + + return rows.Err() } func (c PGStatStatementsCollector) buildExclusionClause(identifiers []string, clauseTemplate string) string { diff --git a/collector/pg_stat_user_tables.go b/collector/pg_stat_user_tables.go index db4e4184..5053a5d1 100644 --- a/collector/pg_stat_user_tables.go +++ b/collector/pg_stat_user_tables.go @@ -419,8 +419,5 @@ func (c *PGStatUserTablesCollector) Update(ctx context.Context, instance *instan ) } - if err := rows.Err(); err != nil { - return err - } - return nil + return rows.Err() } diff --git a/collector/pg_stat_walreceiver.go b/collector/pg_stat_walreceiver.go index 634cfd57..81e2882f 100644 --- a/collector/pg_stat_walreceiver.go +++ b/collector/pg_stat_walreceiver.go @@ -261,8 +261,6 @@ func (c *PGStatWalReceiverCollector) Update(ctx context.Context, instance *insta labels...) } } - if err := rows.Err(); err != nil { - return err - } - return nil + + return rows.Err() } diff --git a/collector/pg_statio_user_indexes.go b/collector/pg_statio_user_indexes.go index 32e0dd4e..0d1c4a61 100644 --- a/collector/pg_statio_user_indexes.go +++ b/collector/pg_statio_user_indexes.go @@ -103,8 +103,6 @@ func (c *PGStatioUserIndexesCollector) Update(ctx context.Context, instance *ins labels..., ) } - if err := rows.Err(); err != nil { - return err - } - return nil + + return rows.Err() } diff --git a/collector/pg_statio_user_tables.go b/collector/pg_statio_user_tables.go index 6dde2e5f..7bb2521a 100644 --- a/collector/pg_statio_user_tables.go +++ b/collector/pg_statio_user_tables.go @@ -192,5 +192,6 @@ func (PGStatIOUserTablesCollector) Update(ctx context.Context, instance *instanc datnameLabel, schemanameLabel, relnameLabel, ) } + return rows.Err() } diff --git a/collector/pg_xlog_location.go b/collector/pg_xlog_location.go index 2d81b4e7..66663322 100644 --- a/collector/pg_xlog_location.go +++ b/collector/pg_xlog_location.go @@ -81,8 +81,6 @@ func (c PGXlogLocationCollector) Update(ctx context.Context, instance *instance, bytes, ) } - if err := rows.Err(); err != nil { - return err - } - return nil + + return rows.Err() }