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
release-23.2: sql: add server.max_open_transactions_per_gateway cluster setting #118933
Conversation
Release note (sql change): Added the server.max_open_transactions_per_gateway cluster setting. When set to a non-negative value, then non-admin users cannot execute a query if the number of transactions open on the current gateway node is already at the configured limit.
28529ad
to
73480f8
Compare
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
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.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andrewbaptist and @sumeerbhola)
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.
LGTM
@@ -547,6 +547,25 @@ func (ex *connExecutor) execStmtInOpenState( | |||
p.noticeSender = res | |||
ih := &p.instrumentation | |||
|
|||
if maxOpen := maxOpenTransactions.Get(&ex.server.cfg.Settings.SV); maxOpen > 0 { | |||
// NB: SQLTxnsOpen does not include internal executor transactions. | |||
if ex.metrics.EngineMetrics.SQLTxnsOpen.Value() > maxOpen { |
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.
It seems a bit odd to use reporting metrics for a guardrail, but I suppose there is no major harm in it. I guess the alternative would be adding a new atomic counter somewhere, which would basically be the same thing?
Backport 1/1 commits from #118781 on behalf of @rafiss.
/cc @cockroachdb/release
informs #110272
Release note (sql change): Added the
server.max_open_transactions_per_gateway cluster setting. When set to a non-negative value, then non-admin users cannot execute a query if the number of transactions open on the current gateway node is already at the configured limit.
Release justification: high priority, low risk change