Merge #134525 #134723 #134919

134525: kvserver: avoid key heap allocation in multiSSTWriter.rolloverSST r=tbg a=sumeerbhola

rolloverSST is called for every key-value pair in the incoming snapshot, and the key parameter was inadvertently escaping to the heap. This was 20% of the number of allocations on a node in the 150 node cluster test.

Epic: none

Release note: None

134723: ui: add Redact option to stmt diag activation modal r=dhartunian a=dhartunian

When users with the `VIEWACTIVITY` role activate statement diagnostics via the modal in SQL Activity, they now have the option to redact the output via a checkbox.

Resolves: #119040
Epic: CRDB-37557

Release note (ui change): when activating statement diagnostics in the DB Console, users now have the option to produce a redacted bundle as output. This bundle will omit sensitive data.

134919: sql: fix FETCH ABSOLUTE 0 cursor handling r=yuzefovich a=yuzefovich

`FETCH ABSOLUTE 0` positions the cursor before the first row, so nothing should be fetched. Previously, this would result in an internal error because we'd be working on unset datums.

Fixes: #131115.

Release note (bug fix): Previously, CockroachDB would encounter an internal error when evaluating `FETCH ABSOLUTE 0` statements, and this is now fixed. The bug has been present since 22.1 version.

Co-authored-by: sumeerbhola <[email protected]>
Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

pkg/kv/kvserver/store_snapshot.go

@@ -265,6 +265,8 @@ func (msstw *multiSSTWriter) initSST(ctx context.Context) error {
 	return nil
 }
 
+// NB: when nextKey is non-nil, do not do anything in this function to cause
+// nextKey at the caller to escape to the heap.
 func (msstw *multiSSTWriter) finalizeSST(ctx context.Context, nextKey *storage.EngineKey) error {
 	currSpan := msstw.currentSpan()
 	if msstw.currSpanIsMVCCSpan() {
@@ -310,32 +312,37 @@ func (msstw *multiSSTWriter) finalizeSST(ctx context.Context, nextKey *storage.E
 	if nextKey != nil {
 		meta := msstw.currSST.Meta
 		encodedNextKey := nextKey.Encode()
+		// Use nextKeyCopy for the remainder of this function. Calling
+		// errors.Errorf with nextKey caused it to escape to the heap in the
+		// caller of finalizeSST (even when finalizeSST was not called), which was
+		// costly.
+		nextKeyCopy := *nextKey
 		if meta.HasPointKeys && storage.EngineKeyCompare(meta.LargestPoint.UserKey, encodedNextKey) > 0 {
 			metaEndKey, ok := storage.DecodeEngineKey(meta.LargestPoint.UserKey)
 			if !ok {
 				return errors.Errorf("multiSSTWriter created overlapping ingestion sstables: sstable largest point key %s > next sstable start key %s",
-					meta.LargestPoint.UserKey, nextKey)
+					meta.LargestPoint.UserKey, nextKeyCopy)
 			}
 			return errors.Errorf("multiSSTWriter created overlapping ingestion sstables: sstable largest point key %s > next sstable start key %s",
-				metaEndKey, nextKey)
+				metaEndKey, nextKeyCopy)
 		}
 		if meta.HasRangeDelKeys && storage.EngineKeyCompare(meta.LargestRangeDel.UserKey, encodedNextKey) > 0 {
 			metaEndKey, ok := storage.DecodeEngineKey(meta.LargestRangeDel.UserKey)
 			if !ok {
 				return errors.Errorf("multiSSTWriter created overlapping ingestion sstables: sstable largest range del %s > next sstable start key %s",
-					meta.LargestRangeDel.UserKey, nextKey)
+					meta.LargestRangeDel.UserKey, nextKeyCopy)
 			}
 			return errors.Errorf("multiSSTWriter created overlapping ingestion sstables: sstable largest range del %s > next sstable start key %s",
-				metaEndKey, nextKey)
+				metaEndKey, nextKeyCopy)
 		}
 		if meta.HasRangeKeys && storage.EngineKeyCompare(meta.LargestRangeKey.UserKey, encodedNextKey) > 0 {
 			metaEndKey, ok := storage.DecodeEngineKey(meta.LargestRangeKey.UserKey)
 			if !ok {
 				return errors.Errorf("multiSSTWriter created overlapping ingestion sstables: sstable largest range key %s > next sstable start key %s",
-					meta.LargestRangeKey.UserKey, nextKey)
+					meta.LargestRangeKey.UserKey, nextKeyCopy)
 			}
 			return errors.Errorf("multiSSTWriter created overlapping ingestion sstables: sstable largest range key %s > next sstable start key %s",
-				metaEndKey, nextKey)
+				metaEndKey, nextKeyCopy)
 		}
 	}
 	// Account for any additional bytes written other than the KV data.

pkg/sql/logictest/testdata/logic_test/cursor

@@ -216,6 +216,10 @@ query II
 FETCH 0 foo
 ----
 
+query II
+FETCH ABSOLUTE 0 foo
+----
+
 query II
 FETCH FIRST foo
 ----

pkg/sql/sql_cursor.go

@@ -225,6 +225,10 @@ func (f *fetchNode) nextInternal(ctx context.Context) (bool, error) {
 			if f.cursor.curRow > f.offset {
 				return false, errBackwardScan
 			}
+			if f.offset == 0 {
+				// ABSOLUTE 0 is positioned before the first row.
+				return false, nil
+			}
 			for f.cursor.curRow < f.offset {
 				more, err := f.cursor.Next(ctx)
 				if !more || err != nil {

pkg/ui/workspaces/cluster-ui/src/api/statementDiagnosticsApi.ts

@@ -54,6 +54,7 @@ export type InsertStmtDiagnosticRequest = {
   minExecutionLatencySeconds?: number;
   expiresAfterSeconds?: number;
   planGist: string;
+  redacted: boolean;
 };
 
 export type InsertStmtDiagnosticResponse = {
@@ -73,6 +74,7 @@ export async function createStatementDiagnosticsReport(
       min_execution_latency: NumberToDuration(req.minExecutionLatencySeconds),
       expires_after: NumberToDuration(req.expiresAfterSeconds),
       plan_gist: req.planGist,
+      redacted: req.redacted,
     }),
   ).then(response => {
     return {

pkg/ui/workspaces/cluster-ui/src/statementsDiagnostics/activateStatementDiagnosticsModal.tsx

@@ -55,6 +55,7 @@ export const ActivateStatementDiagnosticsModal = React.forwardRef<
   const [minExecLatencyUnit, setMinExecLatencyUnit] = useState("milliseconds");
   const [expiresAfter, setExpiresAfter] = useState(15);
   const [traceSampleRate, setTraceSampleRate] = useState(0.01);
+  const [redacted, setRedacted] = useState(false);
 
   const handleSelectChange = (value: string) => {
     setMinExecLatencyUnit(value);
@@ -95,6 +96,7 @@ export const ActivateStatementDiagnosticsModal = React.forwardRef<
       ),
       expiresAfterSeconds: getExpiresAfter(expires, expiresAfter),
       samplingProbability: getTraceSampleRate(conditional, traceSampleRate),
+      redacted: redacted,
     });
     setVisible(false);
   }, [
@@ -108,6 +110,7 @@ export const ActivateStatementDiagnosticsModal = React.forwardRef<
     traceSampleRate,
     filterPerPlanGist,
     selectedPlanGist,
+    redacted,
   ]);
 
   const onCancelHandler = useCallback(() => setVisible(false), []);
@@ -313,6 +316,10 @@ export const ActivateStatementDiagnosticsModal = React.forwardRef<
               />
             </div>
           )}
+          <Divider type="horizontal" />
+          <Checkbox checked={redacted} onChange={() => setRedacted(!redacted)}>
+            <Text>Redact</Text>
+          </Checkbox>
         </Space>
       </ConfigProvider>
     </Modal>

pkg/ui/workspaces/cluster-ui/src/store/statementDiagnostics/statementDiagnostics.sagas.spec.ts

@@ -39,6 +39,7 @@ describe("statementsDiagnostics sagas", () => {
       minExecutionLatencySeconds: minExecLatency,
       expiresAfterSeconds: expiresAfter,
       planGist: planGist,
+      redacted: false,
     };
 
     const insertResponse: InsertStmtDiagnosticResponse = {

pkg/ui/workspaces/db-console/src/redux/statements/statementsSagas.spec.ts

@@ -41,6 +41,7 @@ describe("statementsSagas", () => {
         minExecutionLatencySeconds: minExecLatency,
         expiresAfterSeconds: expiresAfter,
         planGist: planGist,
+        redacted: false,
       };
       const action = createStatementDiagnosticsReportAction(
         insertStmtDiagnosticsRequest,
@@ -72,6 +73,7 @@ describe("statementsSagas", () => {
       minExecutionLatencySeconds: minExecLatency,
       expiresAfterSeconds: expiresAfter,
       planGist: planGist,
+      redacted: false,
     };
     const action = createStatementDiagnosticsReportAction(
       insertStmtDiagnosticsRequest,