Merge #69931 #69945

69931: kvserver: add no-op safeguard to `TransferLeaseTarget` r=aayushshah15 a=aayushshah15

kvserver: add no-op safeguard to `TransferLeaseTarget`

Release justification: adds no-op safeguard

Release note: None

69945: sql: adjust recently added logging events for txn row count guardrails r=yuzefovich a=yuzefovich

**sql: adjust txn rows written/read guardrails a bit**

Release note (ops change): the meaning of the recently introduced
`transaction_rows_written_err` and `transaction_rows_read_err` (as well
as the corresponding `_log` variables) have been adjusted a bit to
indicate the largest number of rows that is still allowed. In other
words, originally reaching the limit would result in an error, and now
only exceeding the limit would.

Release justification: low-risk adjustment to new functionality.

**sql: adjust recently added logging events for txn row count guardrails**

This commit refactors the recently introduced
`CommonTxnRowsLimitDetails` protobuf struct to make it more
user-friendly (since it is used for logging and errors). Namely, the
following changes are made:
- `Limit` is changed to `NumRows` making it easier to understand
- `ViolatesTxnRowsLimitErr` is removed since it is confusing
- `IsRead` is removed too since it is redundant (the types of logged
events contain the necessary "kind" information and this commit
extends the logic of errors to contain the "kind" info too) and is
confusing.

Additionally, we will no longer log twice if `_err` guardrail is reached
after `_log` was reached previously.

Fixes: #69477.

Release note: None (no release since this was introduced)

Release justification: an update to the new functionality.

Co-authored-by: Aayush Shah <aayush.shah15@gmail.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>

docs/generated/eventlog.md

@@ -1963,9 +1963,7 @@ are more statement within the transaction that haven't been executed yet.
 | `PlaceholderValues` | The mapping of SQL placeholders to their values, for prepared statements. | yes |
 | `TxnID` | TxnID is the ID of the transaction that hit the row count limit. | no |
 | `SessionID` | SessionID is the ID of the session that initiated the transaction. | no |
-| `Limit` | Limit indicates the value of the transaction row count limit that was reached. | no |
-| `ViolatesTxnRowsLimitErr` | ViolatesTxnRowsLimitErr if true indicates that 'transaction_rows_{written|read}_err' limit is violated. | no |
-| `IsRead` | IsRead if true indicates that the "rows read" limit is reached and the "rows written" limit otherwise. | no |
+| `NumRows` | NumRows is the number of rows written/read (depending on the event type) by the transaction that reached the corresponding guardrail. | no |
 
 ### `txn_rows_written_limit`
 
@@ -1992,9 +1990,7 @@ been executed yet.
 | `PlaceholderValues` | The mapping of SQL placeholders to their values, for prepared statements. | yes |
 | `TxnID` | TxnID is the ID of the transaction that hit the row count limit. | no |
 | `SessionID` | SessionID is the ID of the session that initiated the transaction. | no |
-| `Limit` | Limit indicates the value of the transaction row count limit that was reached. | no |
-| `ViolatesTxnRowsLimitErr` | ViolatesTxnRowsLimitErr if true indicates that 'transaction_rows_{written|read}_err' limit is violated. | no |
-| `IsRead` | IsRead if true indicates that the "rows read" limit is reached and the "rows written" limit otherwise. | no |
+| `NumRows` | NumRows is the number of rows written/read (depending on the event type) by the transaction that reached the corresponding guardrail. | no |
 
 ## SQL Slow Query Log (Internal)
 
@@ -2088,9 +2084,7 @@ mutation statements within the transaction that haven't been executed yet.
 | `PlaceholderValues` | The mapping of SQL placeholders to their values, for prepared statements. | yes |
 | `TxnID` | TxnID is the ID of the transaction that hit the row count limit. | no |
 | `SessionID` | SessionID is the ID of the session that initiated the transaction. | no |
-| `Limit` | Limit indicates the value of the transaction row count limit that was reached. | no |
-| `ViolatesTxnRowsLimitErr` | ViolatesTxnRowsLimitErr if true indicates that 'transaction_rows_{written|read}_err' limit is violated. | no |
-| `IsRead` | IsRead if true indicates that the "rows read" limit is reached and the "rows written" limit otherwise. | no |
+| `NumRows` | NumRows is the number of rows written/read (depending on the event type) by the transaction that reached the corresponding guardrail. | no |
 
 ### `txn_rows_written_limit_internal`
 
@@ -2118,9 +2112,7 @@ mutation statements within the transaction that haven't been executed yet.
 | `PlaceholderValues` | The mapping of SQL placeholders to their values, for prepared statements. | yes |
 | `TxnID` | TxnID is the ID of the transaction that hit the row count limit. | no |
 | `SessionID` | SessionID is the ID of the session that initiated the transaction. | no |
-| `Limit` | Limit indicates the value of the transaction row count limit that was reached. | no |
-| `ViolatesTxnRowsLimitErr` | ViolatesTxnRowsLimitErr if true indicates that 'transaction_rows_{written|read}_err' limit is violated. | no |
-| `IsRead` | IsRead if true indicates that the "rows read" limit is reached and the "rows written" limit otherwise. | no |
+| `NumRows` | NumRows is the number of rows written/read (depending on the event type) by the transaction that reached the corresponding guardrail. | no |
 
 ## SQL User and Role operations
 

docs/generated/settings/settings-for-tenants.txt

@@ -113,10 +113,10 @@ sql.defaults.results_buffer.size byte size 16 KiB default size of the buffer tha
 sql.defaults.serial_normalization enumeration rowid default handling of SERIAL in table definitions [rowid = 0, virtual_sequence = 1, sql_sequence = 2, sql_sequence_cached = 3]
 sql.defaults.statement_timeout duration 0s default value for the statement_timeout; default value for the statement_timeout session setting; controls the duration a query is permitted to run before it is canceled; if set to 0, there is no timeout
 sql.defaults.stub_catalog_tables.enabled boolean true default value for stub_catalog_tables session setting
-sql.defaults.transaction_rows_read_err integer 0 the limit for the number of rows read by a SQL transaction which - once reached - will fail the transaction (or will trigger a logging event to SQL_INTERNAL_PERF for internal transactions); use 0 to disable
-sql.defaults.transaction_rows_read_log integer 0 the threshold for the number of rows read by a SQL transaction which - once reached - will trigger a logging event to SQL_PERF (or SQL_INTERNAL_PERF for internal transactions); use 0 to disable
-sql.defaults.transaction_rows_written_err integer 0 the limit for the number of rows written by a SQL transaction which - once reached - will fail the transaction (or will trigger a logging event to SQL_INTERNAL_PERF for internal transactions); use 0 to disable
-sql.defaults.transaction_rows_written_log integer 0 the threshold for the number of rows written by a SQL transaction which - once reached - will trigger a logging event to SQL_PERF (or SQL_INTERNAL_PERF for internal transactions); use 0 to disable
+sql.defaults.transaction_rows_read_err integer 0 the limit for the number of rows read by a SQL transaction which - once exceeded - will fail the transaction (or will trigger a logging event to SQL_INTERNAL_PERF for internal transactions); use 0 to disable
+sql.defaults.transaction_rows_read_log integer 0 the threshold for the number of rows read by a SQL transaction which - once exceeded - will trigger a logging event to SQL_PERF (or SQL_INTERNAL_PERF for internal transactions); use 0 to disable
+sql.defaults.transaction_rows_written_err integer 0 the limit for the number of rows written by a SQL transaction which - once exceeded - will fail the transaction (or will trigger a logging event to SQL_INTERNAL_PERF for internal transactions); use 0 to disable
+sql.defaults.transaction_rows_written_log integer 0 the threshold for the number of rows written by a SQL transaction which - once exceeded - will trigger a logging event to SQL_PERF (or SQL_INTERNAL_PERF for internal transactions); use 0 to disable
 sql.defaults.vectorize enumeration on default vectorize mode [on = 0, on = 2, experimental_always = 3, off = 4]
 sql.defaults.zigzag_join.enabled boolean true default value for enable_zigzag_join session setting; allows use of zig-zag join by default
 sql.distsql.max_running_flows integer 500 maximum number of concurrent flows that can be run on a node

docs/generated/settings/settings.html

@@ -118,10 +118,10 @@
 <tr><td><code>sql.defaults.serial_normalization</code></td><td>enumeration</td><td><code>rowid</code></td><td>default handling of SERIAL in table definitions [rowid = 0, virtual_sequence = 1, sql_sequence = 2, sql_sequence_cached = 3]</td></tr>
 <tr><td><code>sql.defaults.statement_timeout</code></td><td>duration</td><td><code>0s</code></td><td>default value for the statement_timeout; default value for the statement_timeout session setting; controls the duration a query is permitted to run before it is canceled; if set to 0, there is no timeout</td></tr>
 <tr><td><code>sql.defaults.stub_catalog_tables.enabled</code></td><td>boolean</td><td><code>true</code></td><td>default value for stub_catalog_tables session setting</td></tr>
-<tr><td><code>sql.defaults.transaction_rows_read_err</code></td><td>integer</td><td><code>0</code></td><td>the limit for the number of rows read by a SQL transaction which - once reached - will fail the transaction (or will trigger a logging event to SQL_INTERNAL_PERF for internal transactions); use 0 to disable</td></tr>
-<tr><td><code>sql.defaults.transaction_rows_read_log</code></td><td>integer</td><td><code>0</code></td><td>the threshold for the number of rows read by a SQL transaction which - once reached - will trigger a logging event to SQL_PERF (or SQL_INTERNAL_PERF for internal transactions); use 0 to disable</td></tr>
-<tr><td><code>sql.defaults.transaction_rows_written_err</code></td><td>integer</td><td><code>0</code></td><td>the limit for the number of rows written by a SQL transaction which - once reached - will fail the transaction (or will trigger a logging event to SQL_INTERNAL_PERF for internal transactions); use 0 to disable</td></tr>
-<tr><td><code>sql.defaults.transaction_rows_written_log</code></td><td>integer</td><td><code>0</code></td><td>the threshold for the number of rows written by a SQL transaction which - once reached - will trigger a logging event to SQL_PERF (or SQL_INTERNAL_PERF for internal transactions); use 0 to disable</td></tr>
+<tr><td><code>sql.defaults.transaction_rows_read_err</code></td><td>integer</td><td><code>0</code></td><td>the limit for the number of rows read by a SQL transaction which - once exceeded - will fail the transaction (or will trigger a logging event to SQL_INTERNAL_PERF for internal transactions); use 0 to disable</td></tr>
+<tr><td><code>sql.defaults.transaction_rows_read_log</code></td><td>integer</td><td><code>0</code></td><td>the threshold for the number of rows read by a SQL transaction which - once exceeded - will trigger a logging event to SQL_PERF (or SQL_INTERNAL_PERF for internal transactions); use 0 to disable</td></tr>
+<tr><td><code>sql.defaults.transaction_rows_written_err</code></td><td>integer</td><td><code>0</code></td><td>the limit for the number of rows written by a SQL transaction which - once exceeded - will fail the transaction (or will trigger a logging event to SQL_INTERNAL_PERF for internal transactions); use 0 to disable</td></tr>
+<tr><td><code>sql.defaults.transaction_rows_written_log</code></td><td>integer</td><td><code>0</code></td><td>the threshold for the number of rows written by a SQL transaction which - once exceeded - will trigger a logging event to SQL_PERF (or SQL_INTERNAL_PERF for internal transactions); use 0 to disable</td></tr>
 <tr><td><code>sql.defaults.vectorize</code></td><td>enumeration</td><td><code>on</code></td><td>default vectorize mode [on = 0, on = 2, experimental_always = 3, off = 4]</td></tr>
 <tr><td><code>sql.defaults.zigzag_join.enabled</code></td><td>boolean</td><td><code>true</code></td><td>default value for enable_zigzag_join session setting; allows use of zig-zag join by default</td></tr>
 <tr><td><code>sql.distsql.max_running_flows</code></td><td>integer</td><td><code>500</code></td><td>maximum number of concurrent flows that can be run on a node</td></tr>

pkg/kv/kvserver/allocator.go

@@ -1451,7 +1451,7 @@ func (a *Allocator) TransferLeaseTarget(
  leaseholderReplQPS, _ := stats.avgQPS()
  currentDelta := getQPSDelta(storeQPSMap, existing)
  bestOption := getCandidateWithMinQPS(storeQPSMap, existing)
- if bestOption != (roachpb.ReplicaDescriptor{}) &&
+ if bestOption != (roachpb.ReplicaDescriptor{}) && bestOption.StoreID != leaseRepl.StoreID() &&
  // It is always beneficial to transfer the lease to the coldest candidate
  // if the range's own qps is smaller than the difference between the
  // leaseholder store and the candidate store. This will always drive down

pkg/kv/kvserver/replicate_queue.go

@@ -1284,10 +1284,15 @@ const (
 )
 
 type transferLeaseOptions struct {
- goal transferLeaseGoal
+ goal transferLeaseGoal
+ // checkTransferLeaseSource, when false, tells `TransferLeaseTarget` to
+ // exclude the current leaseholder from consideration as a potential target
+ // (i.e. when the caller explicitly wants to shed its lease away).
  checkTransferLeaseSource bool
- checkCandidateFullness bool
- dryRun bool
+ // checkCandidateFullness, when false, tells `TransferLeaseTarget`
+ // to disregard the existing lease counts on candidates.
+ checkCandidateFullness bool
+ dryRun bool
 }
 
 // leaseTransferOutcome represents the result of shedLease().

pkg/sql/conn_executor_exec.go

@@ -1095,6 +1095,71 @@ func (ex *connExecutor) dispatchToExecutionEngine(
  return err
 }
 
+type txnRowsWrittenLimitErr struct {
+ eventpb.CommonTxnRowsLimitDetails
+}
+
+var _ error = &txnRowsWrittenLimitErr{}
+var _ errors.SafeDetailer = &txnRowsWrittenLimitErr{}
+var _ fmt.Formatter = &txnRowsWrittenLimitErr{}
+var _ errors.SafeFormatter = &txnRowsWrittenLimitErr{}
+
+// Error is part of the error interface, which txnRowsWrittenLimitErr
+// implements.
+func (e *txnRowsWrittenLimitErr) Error() string {
+ return e.CommonTxnRowsLimitDetails.Error("written")
+}
+
+// SafeDetails is part of the errors.SafeDetailer interface, which
+// txnRowsWrittenLimitErr implements.
+func (e *txnRowsWrittenLimitErr) SafeDetails() []string {
+ return e.CommonTxnRowsLimitDetails.SafeDetails("written")
+}
+
+// Format is part of the fmt.Formatter interface, which txnRowsWrittenLimitErr
+// implements.
+func (e *txnRowsWrittenLimitErr) Format(s fmt.State, verb rune) {
+ errors.FormatError(e, s, verb)
+}
+
+// SafeFormatError is part of the errors.SafeFormatter interface, which
+// txnRowsWrittenLimitErr implements.
+func (e *txnRowsWrittenLimitErr) SafeFormatError(p errors.Printer) (next error) {
+ return e.CommonTxnRowsLimitDetails.SafeFormatError(p, "written")
+}
+
+type txnRowsReadLimitErr struct {
+ eventpb.CommonTxnRowsLimitDetails
+}
+
+var _ error = &txnRowsReadLimitErr{}
+var _ errors.SafeDetailer = &txnRowsReadLimitErr{}
+var _ fmt.Formatter = &txnRowsReadLimitErr{}
+var _ errors.SafeFormatter = &txnRowsReadLimitErr{}
+
+// Error is part of the error interface, which txnRowsReadLimitErr implements.
+func (e *txnRowsReadLimitErr) Error() string {
+ return e.CommonTxnRowsLimitDetails.Error("read")
+}
+
+// SafeDetails is part of the errors.SafeDetailer interface, which
+// txnRowsReadLimitErr implements.
+func (e *txnRowsReadLimitErr) SafeDetails() []string {
+ return e.CommonTxnRowsLimitDetails.SafeDetails("read")
+}
+
+// Format is part of the fmt.Formatter interface, which txnRowsReadLimitErr
+// implements.
+func (e *txnRowsReadLimitErr) Format(s fmt.State, verb rune) {
+ errors.FormatError(e, s, verb)
+}
+
+// SafeFormatError is part of the errors.SafeFormatter interface, which
+// txnRowsReadLimitErr implements.
+func (e *txnRowsReadLimitErr) SafeFormatError(p errors.Printer) (next error) {
+ return e.CommonTxnRowsLimitDetails.SafeFormatError(p, "read")
+}
+
 // handleTxnRowsGuardrails handles either "written" or "read" rows guardrails.
 func (ex *connExecutor) handleTxnRowsGuardrails(
  ctx context.Context,
@@ -1104,46 +1169,30 @@ func (ex *connExecutor) handleTxnRowsGuardrails(
  logCounter, errCounter *metric.Counter,
 ) error {
  var err error
- shouldLog := logLimit != 0 && numRows >= logLimit
- shouldErr := errLimit != 0 && numRows >= errLimit
+ shouldLog := logLimit != 0 && numRows > logLimit
+ shouldErr := errLimit != 0 && numRows > errLimit
  if !shouldLog && !shouldErr {
  return nil
  }
  commonTxnRowsLimitDetails := eventpb.CommonTxnRowsLimitDetails{
  TxnID: ex.state.mu.txn.ID().String(),
  SessionID: ex.sessionID.String(),
- // Limit will be set below.
- ViolatesTxnRowsLimitErr: shouldErr,
- IsRead: isRead,
+ NumRows: numRows,
  }
  if shouldErr && ex.executorType == executorTypeInternal {
  // Internal work should never err and always log if violating either
  // limit.
+ shouldLog = true
  shouldErr = false
- if !shouldLog {
- shouldLog = true
- logLimit = errLimit
- }
  }
  if *alreadyLogged {
  // We have already logged this kind of event about this transaction.
- if shouldErr {
- // But this time we also reached the error limit, so we want to log
- // an event again (it will have ViolatesTxnRowsLimitErr set to
- // true). Note that we couldn't have reached the error limit when we
- // logged the event the previous time because that would have
- // aborted the execution of the transaction.
- shouldLog = true
- logLimit = errLimit
- } else {
- shouldLog = false
- }
+ shouldLog = false
  } else {
  *alreadyLogged = shouldLog
  }
  if shouldLog {
  commonSQLEventDetails := ex.planner.getCommonSQLEventDetails()
- commonTxnRowsLimitDetails.Limit = logLimit
  var event eventpb.EventPayload
  if ex.executorType == executorTypeInternal {
  if isRead {
@@ -1174,8 +1223,12 @@ func (ex *connExecutor) handleTxnRowsGuardrails(
  }
  }
  if shouldErr {
- commonTxnRowsLimitDetails.Limit = errLimit
- err = pgerror.WithCandidateCode(&commonTxnRowsLimitDetails, pgcode.ProgramLimitExceeded)
+ if isRead {
+ err = &txnRowsReadLimitErr{CommonTxnRowsLimitDetails: commonTxnRowsLimitDetails}
+ } else {
+ err = &txnRowsWrittenLimitErr{CommonTxnRowsLimitDetails: commonTxnRowsLimitDetails}
+ }
+ err = pgerror.WithCandidateCode(err, pgcode.ProgramLimitExceeded)
  errCounter.Inc(1)
  }
  return err