-
Notifications
You must be signed in to change notification settings - Fork 499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support/db: Delay canceling queries from client side when there's a statement / transaction timeout configured in postgres #5223
Changes from 3 commits
5065a20
259f224
6036a07
6b4b506
58b5f37
d4e043b
29d392e
fdf63d9
fb56e55
1a874b1
d85eb37
2432bb6
8def1eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -45,38 +45,29 @@ func mustInitHorizonDB(app *App) { | |||||||
log.Fatalf("max open connections to horizon db must be greater than %d", ingest.MaxDBConnections) | ||||||||
} | ||||||||
} | ||||||||
serverSidePGTimeoutConfigs := []db.ClientConfig{ | ||||||||
db.StatementTimeout(app.config.ConnectionTimeout), | ||||||||
db.IdleTransactionTimeout(app.config.ConnectionTimeout), | ||||||||
} | ||||||||
Comment on lines
+48
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the ingestion system uses a separate connection pool go/services/horizon/internal/init.go Lines 87 to 89 in 6036a07
so we can apply these timeout settings to |
||||||||
|
||||||||
if app.config.RoDatabaseURL == "" { | ||||||||
var clientConfigs []db.ClientConfig | ||||||||
if !app.config.Ingest { | ||||||||
// if we are not ingesting then we don't expect to have long db queries / transactions | ||||||||
clientConfigs = append( | ||||||||
clientConfigs, | ||||||||
db.StatementTimeout(app.config.ConnectionTimeout), | ||||||||
db.IdleTransactionTimeout(app.config.ConnectionTimeout), | ||||||||
) | ||||||||
} | ||||||||
app.historyQ = &history.Q{mustNewDBSession( | ||||||||
db.HistorySubservice, | ||||||||
app.config.DatabaseURL, | ||||||||
maxIdle, | ||||||||
maxOpen, | ||||||||
app.prometheusRegistry, | ||||||||
clientConfigs..., | ||||||||
serverSidePGTimeoutConfigs..., | ||||||||
)} | ||||||||
} else { | ||||||||
// If RO set, use it for all DB queries | ||||||||
roClientConfigs := []db.ClientConfig{ | ||||||||
db.StatementTimeout(app.config.ConnectionTimeout), | ||||||||
db.IdleTransactionTimeout(app.config.ConnectionTimeout), | ||||||||
} | ||||||||
app.historyQ = &history.Q{mustNewDBSession( | ||||||||
db.HistorySubservice, | ||||||||
app.config.RoDatabaseURL, | ||||||||
maxIdle, | ||||||||
maxOpen, | ||||||||
app.prometheusRegistry, | ||||||||
roClientConfigs..., | ||||||||
serverSidePGTimeoutConfigs..., | ||||||||
)} | ||||||||
|
||||||||
app.primaryHistoryQ = &history.Q{mustNewDBSession( | ||||||||
|
@@ -85,6 +76,7 @@ func mustInitHorizonDB(app *App) { | |||||||
maxIdle, | ||||||||
maxOpen, | ||||||||
app.prometheusRegistry, | ||||||||
serverSidePGTimeoutConfigs..., | ||||||||
)} | ||||||||
} | ||||||||
} | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just driving by - can we simplify the name to
query-timeout
? I think it delivers the same meaning with less verbosityThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shaptic how about
cancel-query-timeout
? I would like to emphasize that this timeout is only for client side query canceling and not like the postgres server side statement timeoutThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, good point! maybe
client-query-timeout
then? I guess in my mind I read the flag as "cancel query" and think "what the heck is a cancel query, and why does it need a timeout?"(also to be clear I don't want to delay this PR at all)