-
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 6 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 | ||||
---|---|---|---|---|---|---|
|
@@ -437,6 +437,18 @@ func Flags() (*Config, support.ConfigOptions) { | |||||
Usage: "defines the timeout of connection after which 504 response will be sent or stream will be closed, if Horizon is behind a load balancer with idle connection timeout, this should be set to a few seconds less that idle timeout, does not apply to POST /transactions", | ||||||
UsedInCommands: ApiServerCommands, | ||||||
}, | ||||||
&support.ConfigOption{ | ||||||
Name: "cancel-db-query-timeout", | ||||||
ConfigKey: &config.CancelDBQueryTimeout, | ||||||
OptType: types.Int, | ||||||
FlagDefault: 0, | ||||||
CustomSetValue: support.SetDuration, | ||||||
Usage: "defines the timeout for when horizon will cancel all postgres queries connected to an HTTP request. The timeout is measured in seconds since the start of the HTTP request. Note, this timeout does not apply to POST /transactions. " + | ||||||
"The difference between cancel-db-query-timeout and connection-timeout is that connection-timeout applies a postgres statement timeout whereas cancel-db-query-timeout will send an additional request to postgres to cancel the ongoing query. " + | ||||||
"Generally, cancel-db-query-timeout should be configured to be higher than connection-timeout to allow the postgres statement timeout to kill long running queries without having to send the additional cancel request to postgres. " + | ||||||
"By default, cancel-db-query-timeout will be set to twice the connection-timeout.", | ||||||
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.
Suggested change
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. minor, should the flag allow for disabling by the user, I see later in middleware, 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. I didn't want to allow the flag to be disabled because I think it could result in confusion. If we skip the application of this flag, the connection timeout will be set to both the postgres server side statement timeouts and the query cancelations on the horizon side. I think this would be unintuitive because, as a user, my first thought is that disabling the query cancelation timeout would result in no query cancelation whatsoever. If we wanted to allow the option of having no query cancelation at all we could equate 0 to that behavior. -1 could be a default which means cancel-db-query-timeout will be set to twice the connection-timeout value. The user still has the option of setting cancel-db-query-timeout and connection-timeout to the same value which will result in more or less the status quo behavior of horizon prior to this PR. |
||||||
UsedInCommands: ApiServerCommands, | ||||||
}, | ||||||
&support.ConfigOption{ | ||||||
Name: "max-http-request-size", | ||||||
ConfigKey: &config.MaxHTTPRequestSize, | ||||||
|
@@ -983,5 +995,10 @@ func ApplyFlags(config *Config, flags support.ConfigOptions, options ApplyOption | |||||
" If Horizon is behind both, use --behind-cloudflare only") | ||||||
} | ||||||
|
||||||
if config.CancelDBQueryTimeout == 0 { | ||||||
// the default value for cancel-db-query-timeout is twice the connection-timeout | ||||||
config.CancelDBQueryTimeout = config.ConnectionTimeout * 2 | ||||||
} | ||||||
|
||||||
return nil | ||||||
} |
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)