release-25.4: pgwire: make max repeated error count configurable via cluster setting #155658
Conversation
In 39067de we added behavior to give up and close a network connection if a threshold of repeated errors was reached. It retried on errors since some network errors could be transient. It was retrying tens of thousands of times, which is excessive. We lower this to 256 now. This is motivated by a few tests that identifed the error handling logic in this tight loop being quite expensive. Retrying fewer times means that we'll reduce CPU usage during failure scenarios. Release note: None
Previously, the maximum number of repeated network read errors before aborting a connection was a hardcoded constant set to 256 (1 << 8). This change makes the value configurable via a non-public cluster setting `sql.pgwire.max_repeated_error_count`. This allows operators to tune the threshold for aborting connections experiencing repeated network errors, and allows us to backport this change along with 5f562ad. Epic: None Release note: None
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=95b789b1d594ac6b74f377aa7555d8d98d63240d&v=4){kind=avatar}
|
Thanks for opening a backport. Before merging, please confirm that the change does not break backwards compatibility and otherwise complies with the backport policy. Include a brief release justification in the PR description explaining why the backport is appropriate. All backports must be reviewed by the TL for the owning area. While the stricter LTS policy does not yet apply, please exercise judgment and consider gating non-critical changes behind a disabled-by-default feature flag when appropriate. |
blathers-crl
bot
added
backport
|
This change is |
pkg/sql/pgwire/server.go
Outdated
| // while reading from the network connection before the server decides to give | ||
| // up and abort the connection. | ||
| const maxRepeatedErrorCount = 1 << 15 | ||
| const maxRepeatedErrorCount = 1 << 8 |
There was a problem hiding this comment.
you added this commit to the backport, just to prevent merge conflicts, right?
There was a problem hiding this comment.
it looks like in the subsequent commit, the default is 1<<15, which i think makes sense.
There was a problem hiding this comment.
yeah, that was my thinking with this
This is a temporary work around until cockroachdb#155658 is backported to the minor versions that are used by the mixed version test framework. Release note: none Release justification: test only change Fixes: cockroachdb#154125
3 participants
Backport:
Please see individual commits for details.
Release justification: low risk change that is gated by an opt-in cluster setting; will help reduce CPU usage during network timeouts.