sql: reduce syncv2 conn pool usage and add pool utilization metrics #6596

ivan4th · 2025-01-08T16:36:02Z

Motivation

Database.WithConnection is always used by syncv2 for any probe/sync operations against peers, but the actual connection it acquires from the pool is not actually used sometimes, when all the necessary information can be retrieved from the FPTree. Also, while holding the connection for the duration of sync/probe works best in absolute majority of cases, in case of a very slow peer it may cause unwanted extra utilization of the connection pool.

Another problem is that there's currently no metrics for the actual database connection pool utilization, and connection pool wait latency metrics for the main state database pool, api pool and the local database are not collected separately.

Description

This change makes Database.WithConnection acquire the connection from the pool lazily, that is, upon the first query, and release it back if there are no queries executed for certain amount of time (defaults to 10ms), which will prevent slow syncv2 peers from holding connections for too long. In the latter case, the connection is re-acquired from the pool upon the following query.

Also, this change adds connection pool utilization metric, and separates the connection wait latency metrics and pool utilization metrics for th main state database pool, api pool and the local database.

Test Plan

Verify on a mainnet node.

Database.WithConnection is always used by syncv2 for any probe/sync operations against peers, but the actual connection it acquires from the pool is not actually used sometimes, when all the necessary information can be retrieved from the FPTree. Also, while holding the connection for the duration of sync/probe works best in absolute majority of cases, in case of a very slow peer it may cause unwanted extra utilization of the connection pool. Another problem is that there's currently no metrics for the actual database connection pool utilization, and connection pool wait latency metrics for the main state database pool, api pool and the local database are not collected separately. This change makes `Database.WithConnection` acquire the connection from the pool lazily, that is, upon the first query, and release it back if there are no queries executed for certain amount of time (defaults to 10ms), which will prevent slow syncv2 peers from holding connections for too long. In the latter case, the connection is re-acquired from the pool upon the following query. Also, this change adds connection pool utilization metric, and separates the connection wait latency metrics and pool utilization metrics for th main state database pool, api pool and the local database.

codecov · 2025-01-08T17:20:20Z

Codecov Report

Attention: Patch coverage is 99.04762% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.9%. Comparing base (cdee34f) to head (f31ab62).
Report is 2 commits behind head on develop.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
sql/database.go	98.8%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #6596   +/-   ##
=======================================
  Coverage     79.8%   79.9%           
=======================================
  Files          356     356           
  Lines        47339   47417   +78     
=======================================
+ Hits         37820   37902   +82     
+ Misses        7376    7374    -2     
+ Partials      2143    2141    -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fasmat

newLazyCon seems to me quite convoluted. Why does sync2 call WithDBCon when it doesn't need a DB connection, such that WithDBCon then needs additional complexity with the lazy fetching and time based release of the connection?

fasmat · 2025-01-08T17:28:21Z

sql/database.go

+	if conn != nil && db.connWaitLatency != nil {
+		db.connWaitLatency.Observe(time.Since(start).Seconds())
+		db.poolUsage.Inc()
 	}
 	return conn
 }

+func (db *sqliteDatabase) putConn(conn *sqlite.Conn) {
+	db.pool.Put(conn)
+	if db.poolUsage != nil {
+		db.poolUsage.Dec()
+	}
+}
+


Instead of having to check those fields against nil how about setting a default metric name? The only thing this would impact is tests. There we don't read the metrics but collecting them might again affect timings or cause races that we won't detect if we are not using them.

There's also sql.Version() which does not receive any config yet it uses an sqliteDatabase. I've added default database name but if we remove these checks we'll be getting unneeded db metrics entries from a single sql.Version() call early during startup.

fasmat · 2025-01-08T17:29:43Z

sql/database.go

+func (db *sqliteDatabase) putConn(conn *sqlite.Conn) {
+	db.pool.Put(conn)
+	if db.poolUsage != nil {
+		db.poolUsage.Dec()


Won't this poolUsage metric cause a lot of data to be generated? Aren't connections fetched and put back into the pool basically every second at the moment?

It is a gauge. Inc / Dec operations are trivial: https://github.com/prometheus/client_golang/blob/v1.20.5/prometheus/gauge.go#L114-L130

fasmat · 2025-01-08T17:32:08Z

sql/database_test.go

+	numConns := func() int {
+		return int(testutil.ToFloat64(PoolUsage.WithLabelValues(dbName)))
+	}


This can be inlined, if the type check of require.Equal is the reason for this function you can also use require.EqualValues which accepts float64(1) == int(1)

This is done b/c ToFloat64 will always return integer values as float64 here, but if we treat them as such we get a linter error:

./bin/golangci-lint run --config .golangci.yml sql/database_test.go:672:6: float-compare: use require.InEpsilon (or InDelta) (testifylint) require.EqualValues(t, 1, testutil.ToFloat64(PoolUsage.WithLabelValues(dbName))) ^ sql/database_test.go:677:5: float-compare: use require.InEpsilon (or InDelta) (testifylint) require.EqualValues(t, 1, testutil.ToFloat64(PoolUsage.WithLabelValues(dbName))) ^

There's no sense in using require.InEpsilon / require.InDelta here, and silencing the linter warning explicitly is not much better than numConns func, which also makes the intent of reading the metric more clear.

fasmat · 2025-01-08T17:34:33Z

sql/database.go

+	defer func() {
+		c.timer.Reset(c.db.connIdleTimeout)
+	}()


Does c, c.timer, c.db or c.db.connIdleTimeout change between this line and exec returning? If not why not just

Suggested change

defer func() {

c.timer.Reset(c.db.connIdleTimeout)

}()

defer c.timer.Reset(c.db.connIdleTimeout)

fasmat · 2025-01-08T17:40:57Z

sql/database.go

 	conCtx, cancel := context.WithCancel(ctx)
-	defer cancel()
-	conn := db.getConn(conCtx)
-	if conn == nil {
-		return ErrNoConnection
-	}
-	defer db.pool.Put(conn)
-	return exec(&sqliteConn{queryCache: db.queryCache, db: db, conn: conn})
+	c := newLazyConn(conCtx, db)
+	defer func() {
+		cancel()
+		c.release()
+	}()


Why the anonymous function?

conCtx, cancel := context.WithCancel(ctx) defer cancel() c := newLazyConn(conCtx, db) defer c.release()

fasmat · 2025-01-08T17:46:11Z

sql/database.go

+				}
+			}
+		})
+		c.doneCh = make(chan struct{})


I think it would be clearer if getCon inits the timer and the doneCh and releaseCon stops the timer, sets it to nil and also closes the doneCh instead of spreading this across 3 methods.

Which getConn? c.getConn is a function field that's needed so that we don't have to store the context and that's it sole job. sqliteDatabase.getConn probably should not be dealing with doneCh inside the lazyConn.
I don't think that re-creating the timer makes much sense, it may add race with the background goroutine. We could also restart the goroutine, but that's also extra hassle.
And doneCh is only used in the background goroutine in ensureConn and in release where it tells the goroutine to stop when we don't need this particular lazyConn anymore.

ivan4th · 2025-01-08T18:05:16Z

newLazyCon seems to me quite convoluted. Why does sync2 call WithDBCon when it doesn't need a DB connection, such that WithDBCon then needs additional complexity with the lazy fetching and time based release of the connection?

When a sync/probe operation against a peer begins, DBSet.WithCopy method is used to create a temporary shallow (very fast) copy of a DBSet that only lives for the duration of execution of the callback passed to DBSet.WithCopy. This copy of DBSet may or may not need to execute database queries during sync/probe and this cannot be known ahead of time before the actual sync interaction begins. To provide the request-scoped DBSet clone with a database connection, DBSet.WithCopy uses Database.WithConnection. This needs to be done b/c if we just pass the database itself as the Executor, a connection will be retrieved from the pool before executing each SQL query, and put back to the pool after each query, and sometimes syncv2 needs to do tens of thousands of trivial queries, and SQLite pool latency adds up to substantial delays in this case, even if very few connections are used. We could also use a read transaction, but this would add extra burden b/c long-running read transactions prevent SQLite from doing WAL checkpointing, leading to unneeded growth of the WAL file, and syncv2 has a rowid-based "snapshotting" mechanism which makes explicit read transactions unnecessary. So, WithConnection is like "WithTransaction without the actual transaction".

As noted in the PR description, this approach has its downsides, namely that sometimes the allocated connections are not actually used (which is impossible to predict at the point in which WithConnection is called) and also that a slow peer may hold a connection for too long, making it more likely for the pool to be exhausted. In theory, the logic to alleviate these issues, namely, lazy conn acquisition and putting idle conns back to the pool could be done on the syncv2 side, but I think that the resulting code will be much more convoluted.

ivan4th requested review from dshulyak, fasmat, poszu, jellonek and kacpersaw as code owners January 8, 2025 16:36

ivan4th force-pushed the sql/release-conns branch from bec23c3 to 8496629 Compare January 8, 2025 16:37

fasmat reviewed Jan 8, 2025

View reviewed changes

ivan4th added 2 commits January 8, 2025 23:19

sql: address comments and fix race

93098a8

sql: address comments

f31ab62

ivan4th added the area/syncv2 label Jan 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: reduce syncv2 conn pool usage and add pool utilization metrics #6596

sql: reduce syncv2 conn pool usage and add pool utilization metrics #6596

ivan4th commented Jan 8, 2025

codecov bot commented Jan 8, 2025 •

edited

Loading

fasmat left a comment •

edited

Loading

fasmat Jan 8, 2025

ivan4th Jan 8, 2025

fasmat Jan 8, 2025

ivan4th Jan 8, 2025

fasmat Jan 8, 2025

ivan4th Jan 8, 2025

fasmat Jan 8, 2025

ivan4th Jan 8, 2025

fasmat Jan 8, 2025

ivan4th Jan 8, 2025

fasmat Jan 8, 2025

ivan4th Jan 8, 2025

ivan4th commented Jan 8, 2025 •

edited

Loading

sql: reduce syncv2 conn pool usage and add pool utilization metrics #6596

Are you sure you want to change the base?

sql: reduce syncv2 conn pool usage and add pool utilization metrics #6596

Conversation

ivan4th commented Jan 8, 2025

Motivation

Description

Test Plan

codecov bot commented Jan 8, 2025 • edited Loading

Codecov Report

fasmat left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan4th commented Jan 8, 2025 • edited Loading

codecov bot commented Jan 8, 2025 •

edited

Loading

fasmat left a comment •

edited

Loading

ivan4th commented Jan 8, 2025 •

edited

Loading