Merge #148001 #148700 #148743

148001: server,ui: increase max result size for sql over http requests r=dhartunian a=kyle-a-wong

The default for max result size for the sql over http v2 API was previously set to 10kb, which was frequently resulting in incomplete results / error messages in db console. Additionally, db console used 50kb to as the
"LARGE_RESULT_SIZE" value for many of db consoles sql over http requests. This has also been seen to result in errors like "Not all insights are displayed because the maximum number of insights was reached in the console." in the db console insights page.

This change increases the default on the server side from 10kb to 100kb and the value of "LARGE_RESULT_SIZE" from 50kb to 1Mb

Resolves: #146875
Epic: None
Release note: None

148700: sql: always wait for all goroutines in fingerprintSpan r=yuzefovich a=yuzefovich

This commit fixes an oversight from 1cf6236. In particular, in that change we ensured that we never leak the worker goroutines in fingerprintSpan by cancelling the context in the coordinator and checking for that in the workers. However, it was possible for the coordinator to exit before the workers which can lead to undefined behavior down the line (in a test failure we just saw usage of the trace span after finish). This commit fixes the oversight by always blocking the coordinator until all its workers exit.

Fixes: #148602.

Release note: None

148743: sql: version-gate sql.txn.write_buffering_for_weak_isolation.enabled r=yuzefovich a=stevendanna

Buffering writes in weak isolation levels such as read committed requires lock loss detection to avoid intra-statement anomalies.

Lock loss detection is only available when all nodes are on 25.3 or greater.

Epic: none
Release note: None

Co-authored-by: Kyle Wong <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Steven Danna <[email protected]>

Author: 4 people

pkg/ccl/logictestccl/testdata/logic_test/buffered_writes_lock_loss

@@ -1,4 +1,4 @@
-# LogicTest: !3node-tenant
+# LogicTest: !3node-tenant !local-mixed-25.2
 # cluster-opt: disable-mvcc-range-tombstones-for-point-deletes
 
 statement ok

pkg/ccl/logictestccl/tests/local-mixed-25.2/BUILD.bazel

@@ -9,7 +9,7 @@ go_test(
         "//pkg/ccl/logictestccl:testdata",  # keep
     ],
     exec_properties = {"test.Pool": "large"},
-    shard_count = 34,
+    shard_count = 33,
     tags = ["cpu:1"],
     deps = [
         "//pkg/base",

pkg/ccl/logictestccl/tests/local-mixed-25.2/generated_test.go

@@ -12,7 +12,7 @@ sql admin
   "application_name": "$ api-v2-sql",
   "database": "system",
   "execute": false,
-  "max_result_size": 10000,
+  "max_result_size": 100000,
   "separate_txns": false,
   "statements": [
    {

pkg/ccl/serverccl/testdata/api_v2_sql

@@ -306,7 +306,8 @@ func (a *apiV2Server) execSQL(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	if requestPayload.MaxResultSize == 0 {
-		requestPayload.MaxResultSize = 10000
+		// Default to 100 kb if no MaxResultSize is provided.
+		requestPayload.MaxResultSize = 100_000
 	}
 	if len(requestPayload.Statements) == 0 {
 		topLevelError(errors.New("no statements specified"), http.StatusBadRequest)

pkg/server/api_v2_sql.go

@@ -10,7 +10,7 @@ sql admin
   "application_name": "$ api-v2-sql",
   "database": "system",
   "execute": false,
-  "max_result_size": 10000,
+  "max_result_size": 100000,
   "separate_txns": false,
   "statements": [
    {

pkg/server/testdata/api_v2_sql

@@ -30,6 +30,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/settings"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/appstatspb"
 	"github.com/cockroachdb/cockroach/pkg/sql/auditlogging"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
@@ -3543,9 +3544,7 @@ func (ex *connExecutor) setTransactionModes(
 		if err := ex.state.setIsolationLevel(level); err != nil {
 			return pgerror.WithCandidateCode(err, pgcode.ActiveSQLTransaction)
 		}
-		if (level != isolation.Serializable) && !allowBufferedWritesForWeakIsolation.Get(&ex.server.cfg.Settings.SV) {
-			// TODO(#143497): we currently only support buffered writes under
-			// serializable isolation.
+		if !ex.bufferedWritesIsAllowedForIsolationLevel(ctx, level) {
 			ex.state.mu.txn.SetBufferedWritesEnabled(false)
 		}
 	}
@@ -3731,6 +3730,28 @@ func (ex *connExecutor) bufferedWritesEnabled(ctx context.Context) bool {
 	return ex.sessionData().BufferedWritesEnabled && ex.server.cfg.Settings.Version.IsActive(ctx, clusterversion.V25_2)
 }
 
+func (ex *connExecutor) bufferedWritesIsAllowedForIsolationLevel(
+	ctx context.Context, isoLevel isolation.Level,
+) bool {
+	return bufferedWritesIsAllowedForIsolationLevel(ctx, ex.server.cfg.Settings, isoLevel)
+}
+
+func bufferedWritesIsAllowedForIsolationLevel(
+	ctx context.Context, st *cluster.Settings, isoLevel isolation.Level,
+) bool {
+	if isoLevel == isolation.Serializable {
+		return true
+	}
+
+	// We are at a weaker isolation level that requires lock loss detection which
+	// is only available on 25.3 or greater.
+	if !st.Version.IsActive(ctx, clusterversion.V25_3) {
+		return false
+	}
+
+	return allowBufferedWritesForWeakIsolation.Get(&st.SV)
+}
+
 // initEvalCtx initializes the fields of an extendedEvalContext that stay the
 // same across multiple statements. resetEvalCtx must also be called before each
 // statement, to reinitialize other fields.

pkg/sql/conn_executor.go

@@ -133,15 +133,12 @@ func (p *planner) fingerprintSpanFanout(
 	fingerprintPartition := func(
 		partition roachpb.Spans,
 	) func(ctx context.Context) error {
-		return func(ctx context.Context) error {
+		return func(ctx context.Context) (retErr error) {
 			// workCh is used to divide up the partition between workers. It is
 			// closed whenever there is no work to do. It might not be closed if
 			// the coordinator encounters an error.
 			workCh := make(chan roachpb.Span)
-			// The context will be canceled when the coordinator exits
-			// guaranteeing that the worker goroutines aren't leaked.
 			ctx, cancel := context.WithCancel(ctx)
-			defer cancel()
 
 			grp := ctxgroup.WithContext(ctx)
 			for range maxWorkerCount {
@@ -169,6 +166,27 @@ func (p *planner) fingerprintSpanFanout(
 					}
 				})
 			}
+			defer func() {
+				// Either workCh is closed (meaning that we've processed all the
+				// work), or we hit an error in the loop below. If it's the
+				// latter, we need to cancel the context to signal to the
+				// workers to shutdown ASAP.
+				if retErr != nil {
+					cancel()
+				}
+				// Regardless of how we got here, ensure that we always block
+				// until all workers exit.
+				// TODO(yuzefovich): refactor the logic here so that the
+				// coordinator goroutine had a single return point. This will
+				// also allow us to prevent a hypothetical scenario where we're
+				// blocked forever (i.e. until the context is canceled) writing
+				// into workCh which can happen if all worker goroutines exit
+				// due to an error.
+				grpErr := grp.Wait()
+				if retErr == nil {
+					retErr = grpErr
+				}
+			}()
 
 			for _, part := range partition {
 				rdi, err := p.execCfg.RangeDescIteratorFactory.NewLazyIterator(ctx, part, 64)
@@ -195,7 +213,7 @@ func (p *planner) fingerprintSpanFanout(
 				}
 			}
 			close(workCh)
-			return grp.Wait()
+			return nil
 		}
 	}
 

pkg/sql/fingerprint_span.go

@@ -252,9 +252,7 @@ func (ts *txnState) resetForNewSQLTxn(
 			if err := ts.setIsolationLevelLocked(isoLevel); err != nil {
 				panic(err)
 			}
-			if isoLevel != isolation.Serializable && !allowBufferedWritesForWeakIsolation.Get(&tranCtx.settings.SV) {
-				// TODO(#143497): we currently only support buffered writes
-				// under serializable isolation.
+			if !bufferedWritesIsAllowedForIsolationLevel(connCtx, tranCtx.settings, isoLevel) {
 				bufferedWritesEnabled = false
 			}
 			if bufferedWritesEnabled {

pkg/sql/txn_state.go

@@ -10,7 +10,7 @@ import { RequestError } from "../util";
 
 import {
   executeInternalSql,
-  MAX_RESULT_SIZE,
+  LARGE_RESULT_SIZE,
   SqlExecutionRequest,
   sqlResultsAreEmpty,
 } from "./sqlApi";
@@ -74,7 +74,7 @@ export function getSchedules(req: {
         arguments: args,
       },
     ],
-    max_result_size: MAX_RESULT_SIZE,
+    max_result_size: LARGE_RESULT_SIZE,
     execute: true,
   };
   return executeInternalSql<ScheduleColumns>(request).then(result => {