chore: one (*sql.DB) pool for all jobsdb #5170

Sidddddarth · 2024-10-04T07:02:44Z

Description

One connection pool for all jobsdb.
In future, use the same pool across other areas(reporting, backend-config cache, drain-config etc. with appropriate increase in pool configurations)

Motivation

There's several problems to deal with
By default each jobsdb connection pool could only hold two idle connections, this meant creating a lot of new connections often
It was also observed that the overall in_use connections at any point of time was only around 20-25(this includes reporting, config cache etc.), there was rarely a need for more connections to the database. Also observed was the fact that having too many idle connections lying around caused unnecessary memory usage on postgres pods. A better control over the idle and total connections in use would be great.

So considering these factors, it made sense to have a common connection pool and have different components borrow from it.

To allow for a surge in connection demand, the max open connections config is at a generous distance, which is configurable of course in case 40(default) isn't enough.

Configuration

Using given config(all of these are hot-reloadable):
maxOpenConnections: 40
maxIdleConnections: 5
maxIdleConnIdleTime: 15 Minutes
maxConnLifetime: 0

Linear Ticket

Resolves PIPE-1602

Security

The code changed/added as part of this pull request won't create any security issues with how the software is being used.

codecov · 2024-10-04T07:33:18Z

Codecov Report

Attention: Patch coverage is 63.72549% with 37 lines in your changes missing coverage. Please review.

Project coverage is 73.15%. Comparing base (72443b2) to head (55bfc68).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
utils/misc/dbutils.go	63.63%	13 Missing and 3 partials ⚠️
app/apphandlers/processorAppHandler.go	0.00%	13 Missing ⚠️
jobsdb/jobsdb.go	81.81%	2 Missing and 2 partials ⚠️
app/apphandlers/embeddedAppHandler.go	86.66%	1 Missing and 1 partial ⚠️
app/apphandlers/gatewayAppHandler.go	75.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5170      +/-   ##
==========================================
+ Coverage   72.91%   73.15%   +0.24%     
==========================================
  Files         439      439              
  Lines       51134    51223      +89     
==========================================
+ Hits        37282    37473     +191     
+ Misses      11390    11312      -78     
+ Partials     2462     2438      -24

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

utils/misc/dbutils.go

lvrach · 2024-10-07T14:22:04Z

app/apphandlers/embeddedAppHandler.go

+ // Not deferring close here
+ // defer dbPool.Close()


What the reason for not closing here? Shall we inject dbPool to embeddedAppHandler. So we always use on connection pool

just to keep the existing behaviour. We don't close gateway db pool, but the others are closed right away. Since we're sharing the pool now, didn't want to close it.
Just left this comment to find out why the gateway pool wasn't being closed earlier because it's closed in gateway app handler anyway.

With the way things are, injecting a pool into apps would be a good idea. This would effectively enforce the same database for all components. But I think it'd be nice to have the option of passing different connections pools to different components.

what is the existing behaviour you are referring to? I see that every component was closing itself, why have we removed that?

we close all *sql.DB except gateway db. gateway*jobsdb.Handle is only Stopped.

in any case, closing it everywhere
looks like there was no reason after all

app/apphandlers/embeddedAppHandler.go

Co-authored-by: Leonidas Vrachnis <leo.al.vra@gmail.com>

mihir20

@Sidddddarth can you add more details in description on why we are going with single db connection pool?

lvrach · 2024-10-08T08:47:30Z

Can we introduce a feature flag to control whether we want a shared connection pool or not? Once this PR gets merged we will have no way to switch to the previous behaviour, but to revert.

I would suggest passing nil db connection, as a signal to components that they need to create there own connections

app/apphandlers/embeddedAppHandler.go

BonapartePC · 2024-10-08T17:33:51Z

utils/misc/dbutils.go

+ updatePoolConfig(db.SetMaxOpenConns, &maxConns, maxConnsVar)
+ updatePoolConfig(db.SetConnMaxIdleTime, &maxIdleTime, maxIdleTimeVar)
+ updatePoolConfig(db.SetMaxIdleConns, &maxIdleConns, maxIdleConnsVar)
+ updatePoolConfig(db.SetConnMaxLifetime, &maxConnLifetime, maxConnLifetimeVar)


Why not move this below <-ticker.C?

no reason
just used to writing it like this

BonapartePC · 2024-10-08T17:51:06Z

utils/misc/dbutils.go

+ return db, nil
+}
+
+func updatePoolConfig[T comparable](f func(T), current *T, conf config.ValueLoader[T]) {


Suggested change

func updatePoolConfig[T comparable](f func(T), current *T, conf config.ValueLoader[T]) {

func updatePoolConfig[T comparable](setter func(T), current *T, conf config.ValueLoader[T]) {

[Optional]

ktgowtham

LGTM 👍

lvrach

We need to modify

rudder-server/jobsdb/jobsdb.go

Line 1094 in 209a9c1

func (jd *Handle) Close() {

So that db closes the connection only if the connection was initialised from jobsdb.

lvrach · 2024-10-09T12:49:32Z

jobsdb/jobsdb.go

@@ -769,7 +770,9 @@ func (jd *Handle) init() {
 jd.loadConfig()

 // Initialize dbHandle if not already set
- if jd.dbHandle == nil {
+ if jd.dbHandle != nil {
+ jd.sharedConnectionPool = true


We can also do:

Suggested change

jd.sharedConnectionPool = true

jd.onCloseFn = func () {

if err := jd.dbHandle.Close(); err != nil {

jd.logger.Errorw("error closing db connection", "error", err)

}

}

chore: one (*sql.DB) pool for all jobsdb

3966164

Sidddddarth and others added 2 commits October 4, 2024 19:47

chore: update default values, return err if stat register fails

163c085

defer db.Close() in gateway app handler

625f3ee

Sidddddarth marked this pull request as ready for review October 7, 2024 03:49

Sidddddarth added 2 commits October 7, 2024 09:19

Merge branch 'master' into chore.oneConnPoolForAllJobsdb

6b85d6f

Merge branch 'master' into chore.oneConnPoolForAllJobsdb

dfa116a

ktgowtham requested review from ktgowtham, BonapartePC and lvrach October 7, 2024 11:05

lvrach reviewed Oct 7, 2024

View reviewed changes

Sidddddarth and others added 2 commits October 8, 2024 09:24

Apply suggestions from code review

310aa7c

Co-authored-by: Leonidas Vrachnis <leo.al.vra@gmail.com>

fixup! Apply suggestions from code review

12df2fc

Sidddddarth force-pushed the chore.oneConnPoolForAllJobsdb branch from 4d6cbab to 12df2fc Compare October 8, 2024 05:44

mihir20 reviewed Oct 8, 2024

View reviewed changes

fixup! Apply suggestions from code review

18a0b29

Sidddddarth requested review from lvrach and mihir20 October 8, 2024 10:35

BonapartePC reviewed Oct 8, 2024

View reviewed changes

app/apphandlers/embeddedAppHandler.go Show resolved Hide resolved

Update embeddedAppHandler.go

bc9b051

BonapartePC reviewed Oct 8, 2024

View reviewed changes

Sidddddarth and others added 4 commits October 9, 2024 06:05

Merge branch 'master' into chore.oneConnPoolForAllJobsdb

618e99c

chore: test to verify results in both modes

14b50c7

Update docker_test.go

7464400

address comments

209a9c1

BonapartePC approved these changes Oct 9, 2024

View reviewed changes

ktgowtham approved these changes Oct 9, 2024

View reviewed changes

lvrach reviewed Oct 9, 2024

View reviewed changes

chore: jobsdb Handle closes dbHandle only if it's not shared

55bfc68

lvrach reviewed Oct 9, 2024

View reviewed changes

lvrach approved these changes Oct 9, 2024

View reviewed changes

Sidddddarth enabled auto-merge (squash) October 9, 2024 13:03

Sidddddarth merged commit 261aa60 into master Oct 9, 2024
55 checks passed

Sidddddarth deleted the chore.oneConnPoolForAllJobsdb branch October 9, 2024 13:07

This was referenced Oct 14, 2024

chore: release 1.36.0 #5193

Closed

chore: prerelease 1.36.0-rc.1 #5194

Closed

chore: prerelease 1.36.0-rc.1 #5195

Merged

chore: release 1.36.0 #5196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: one (*sql.DB) pool for all jobsdb #5170

chore: one (*sql.DB) pool for all jobsdb #5170

Sidddddarth commented Oct 4, 2024 •

edited

Loading

codecov bot commented Oct 4, 2024 •

edited

Loading

lvrach Oct 7, 2024

Sidddddarth Oct 8, 2024

Sidddddarth Oct 8, 2024

lvrach Oct 8, 2024

Sidddddarth Oct 8, 2024 •

edited

Loading

Sidddddarth Oct 8, 2024

mihir20 left a comment

lvrach commented Oct 8, 2024

BonapartePC Oct 8, 2024

Sidddddarth Oct 9, 2024

BonapartePC Oct 8, 2024

ktgowtham left a comment

lvrach left a comment

lvrach Oct 9, 2024

	func updatePoolConfig[T comparable](f func(T), current *T, conf config.ValueLoader[T]) {
	func updatePoolConfig[T comparable](setter func(T), current *T, conf config.ValueLoader[T]) {

- jd.sharedConnectionPool = true
+ jd.onCloseFn = func () {
+ if err := jd.dbHandle.Close(); err != nil {
+ jd.logger.Errorw("error closing db connection", "error", err)
+ }
+ }

chore: one (*sql.DB) pool for all jobsdb #5170

chore: one (*sql.DB) pool for all jobsdb #5170

Conversation

Sidddddarth commented Oct 4, 2024 • edited Loading

Description

Motivation

Configuration

Linear Ticket

Security

codecov bot commented Oct 4, 2024 • edited Loading

Codecov Report

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

Sidddddarth Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihir20 left a comment

Choose a reason for hiding this comment

lvrach commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktgowtham left a comment

Choose a reason for hiding this comment

lvrach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sidddddarth commented Oct 4, 2024 •

edited

Loading

codecov bot commented Oct 4, 2024 •

edited

Loading

Sidddddarth Oct 8, 2024 •

edited

Loading