sql: add server.max_open_transactions_per_gateway cluster setting #118781
Conversation
rafiss
added
backport-23.1.x
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
rafiss
force-pushed
the
max-active-per-gateway
branch
from
d68dd54 to
201bab3
Compare
There was a problem hiding this comment.
I don't have enough context to review this.
Is there any recommendation for how cluster operators can set this prior to an outage? Should they be looking at the max value of sql.txns.open over a long time duration and use that to guide what they set this to?
Can they use this during an outage -- will it effect transactions that are already open? If not, is the hope that enough transactions are completing that it will soon have an effect?
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist)
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status:
pkg/sql/conn_executor_exec.go line 551 at r1 (raw file):
ih := &p.instrumentation if maxOpen := maxOpenTransactions.Get(&ex.server.cfg.Settings.SV); maxOpen > 0 {
Unrelated and more of a comment, but I can imagine queuing behaviour might be preferred in certain cases.
I think looking at Another heuristic is our guidance that says that the number of actively running transactions should not greatly exceed
Yes. The check and error would occur as soon as the next statement in a transaction begins executing, after this cluster setting is applied. I can add a test for that. |
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.
rafiss
force-pushed
the
max-active-per-gateway
branch
from
201bab3 to
a07fb8d
Compare
|
tftr! bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from a07fb8d to blathers/backport-release-23.1-118781: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
@rafiss friendly ping to manually backport this to 23.1 -- the blathers backport failed. We had an escalation (https://github.com/cockroachlabs/support/issues/2812) where this could have been useful. |
Previously this test was created but wasn't run since it would always fail. The test still doesn't pass in the default configuration, however after the addition of cockroachdb#118781, at least it is possible to configure a cluster to pass. Informs: cockroachdb#110272 Informs: cockroachdb#89142 Release note: None
121833: roachtest: update tpcc overload r=rafiss a=andrewbaptist Previously this test was created but wasn't run since it would always fail. The test still doesn't pass in the default configuration, however after the addition of #118781, at least it is possible to configure a cluster to pass. Informs: #110272 Informs: #89142 Release note: None Co-authored-by: Andrew Baptist <[email protected]>
| @@ -548,6 +548,25 @@ func (ex *connExecutor) execStmtInOpenState( | |||
| p.noticeSender = res | |||
| ih := &p.instrumentation | |||
|
|
|||
| if maxOpen := maxOpenTransactions.Get(&ex.server.cfg.Settings.SV); maxOpen > 0 { | |||
There was a problem hiding this comment.
What is the behavior when server.max_open_transactions_per_gateway is zero? Seems like it's still unlimited?
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.