Merge #151737 #151880 #151951 #152033

151737: sql: add support for automatically repairing dangling comments r=fqazi a=fqazi

Previously, we added logic to detect dangling comments on descriptors but did not include a mechanism to clean them up. This could block initial upgrades due to dangling entries in the system.comments table. This patch adds support for automatically cleaning up these dangling comments.

Fixes: #151497

Release note (bug fix): Added an automatic repair for dangling or invalid entries in the system.comment table.

151880: server: Filter high contention insights in ListExecutionInsights r=alyshanjahani-crl a=alyshanjahani-crl

Previously high contention insights included in the ListExecutionInsights response often did not have their respective contention events available in the contention events registry. This led to confusion when viewing an insight execution overview page since the table displaying the contention events for the execution would often be empty.

This commit fixes this behaviour by ensuring that any high contention insights are only included in the ListExecutionInsights response if they have at least one valid contention event available in the contention event registry.

A contention event is valid if the blocking transaction fingerprint ID (i.e. the column of most interest in the UI table) is resolved.

Fixes: https://cockroachlabs.atlassian.net/browse/CRDB-53271
Release note: None

151951: sql: fix `BETWEEN SYMMETRIC` normalization to use matching typed lefts r=Uzair5162 a=Uzair5162

We normalize symmetric `RangeCond` expressions (`<left> BETWEEN SYMMETRIC <from> AND <to>`) by expanding them into 2 pairs of `ComparisonExpr`, covering both orderings of the BETWEEN bounds. When the type-checks for the left/from and left/to comparisons chose different coercions, the symmetric half could pair `leftFrom` with `to` (and `leftTo` with `from`) during normalization. This resulted in mixed-type comparisons on type-checked expressions and caused panics in some paths (e.g. AOST normalization).

This change ensures that the type-checked pairings are kept aligned when constructing `ComparisonExpr` in the symmetric case.

Fixes: #133395

Release note (bug fix): Previously, executing certain statements with `BETWEEN SYMMETRIC` expressions could panic if used with values of different types, such as `... b'bytes' BETWEEN SYMMETRIC 'a' AND 'c'`, which is now fixed.

152033: sql/stats: ignore partial stats in `crdb_internal.table_row_statistics` r=Uzair5162 a=Uzair5162

Previously, the `crdb_internal.table_row_statistics` virtual table was being populated with the row count from the most recent table statistic. This caused incorrect row counts to be shown for `SHOW TABLES` when the most recent statistic was a partial collection. This change ignores partial stats when populating `crdb_internal.table_row_statistics` to fix this issue.

Fixes: #152024

Release note (bug fix): Previously, `SHOW TABLES` would show inaccurate row counts if the most recent statistic collection was partial, which is now fixed.

Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Alyshan Jahani <[email protected]>
Co-authored-by: Uzair Ahmad <[email protected]>

Author: 4 people

pkg/server/BUILD.bazel

@@ -556,6 +556,8 @@ go_test(
         "//pkg/sql/catalog/descs",
         "//pkg/sql/catalog/desctestutils",
         "//pkg/sql/catalog/systemschema",
+        "//pkg/sql/contention",
+        "//pkg/sql/contentionpb",
         "//pkg/sql/execinfrapb",
         "//pkg/sql/isql",
         "//pkg/sql/pgwire",

pkg/server/status.go

@@ -61,6 +61,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/spanconfig"
 	"github.com/cockroachdb/cockroach/pkg/sql"
+	"github.com/cockroachdb/cockroach/pkg/sql/appstatspb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
 	"github.com/cockroachdb/cockroach/pkg/sql/clusterunique"
 	"github.com/cockroachdb/cockroach/pkg/sql/contention"
@@ -86,6 +87,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb"
 	"github.com/cockroachdb/cockroach/pkg/util/uint128"
+	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/redact"
 	"github.com/google/pprof/profile"
@@ -393,23 +395,70 @@ func (b *baseStatusServer) localExecutionInsights(
 ) (*serverpb.ListExecutionInsightsResponse, error) {
 	var response serverpb.ListExecutionInsightsResponse
 
+	highContentionInsights := make(map[uuid.UUID]insights.Insight, 0)
 	reader := b.sqlServer.pgServer.SQLServer.GetInsightsReader()
 	reader.IterateInsights(ctx, func(ctx context.Context, insight *insights.Insight) {
 		if insight == nil {
 			return
 		}
 
-		insightsCopy := *insight
 		// Copy statements slice - these insights objects can be read concurrently.
+		insightsCopy := *insight
 		insightsCopy.Statements = make([]*insights.Statement, len(insight.Statements))
 		copy(insightsCopy.Statements, insight.Statements)
-
-		response.Insights = append(response.Insights, insightsCopy)
+		if insight.Transaction != nil && slices.Contains(insight.Transaction.Causes, insights.Cause_HighContention) {
+			// Collect high contention insights seperately, they need additional validation / filtering.
+			// If it is valid we will add it to the response later.
+			highContentionInsights[insightsCopy.Transaction.ID] = insightsCopy
+		} else {
+			// All other insights are included in the response.
+			response.Insights = append(response.Insights, insightsCopy)
+		}
 	})
 
+	// Validating contention insights involves iterating through the contention events registry.
+	// Only do so if we have insights to validate.
+	if len(highContentionInsights) > 0 {
+		// Include only the valid high contention insights in the response.
+		valid, err := validContentionInsights(b.sqlServer.execCfg.ContentionRegistry, highContentionInsights)
+		if err != nil {
+			return nil, errors.Wrap(err, "validating contention insights")
+		}
+
+		for validInsightTxnID := range valid {
+			response.Insights = append(response.Insights, highContentionInsights[validInsightTxnID])
+		}
+	}
+
 	return &response, nil
 }
 
+// validContentionInsights iterates through the contention event registry and checks the
+// events that are related to the passed in (contention) insights. The insights that have
+// valid (resolved BlockingTxnFingerprintID) contention events are returned.
+func validContentionInsights(
+	registry *contention.Registry, contentionInsights map[uuid.UUID]insights.Insight,
+) (map[uuid.UUID]bool, error) {
+	valid := make(map[uuid.UUID]bool, len(contentionInsights))
+	err := registry.ForEachEvent(func(event *contentionpb.ExtendedContentionEvent) error {
+		// If we have already determined an execution (insight) to be valid, no work to do.
+		if ok := valid[event.WaitingTxnID]; ok {
+			return nil
+		}
+		// We are interested in the contention events that are related to our insights.
+		if _, ok := contentionInsights[event.WaitingTxnID]; ok {
+			// If the event has the blocking transaction fingerprint ID resolved, we consider it
+			// valid for the insights response (since the insight response already has the waiting
+			// transaction fingerprint ID).
+			if event.BlockingTxnFingerprintID != appstatspb.InvalidTransactionFingerprintID {
+				valid[event.WaitingTxnID] = true
+			}
+		}
+		return nil
+	})
+	return valid, err
+}
+
 func (b *baseStatusServer) localTxnIDResolution(
 	req *serverpb.TxnIDResolutionRequest,
 ) *serverpb.TxnIDResolutionResponse {

pkg/server/status_test.go

@@ -25,7 +25,10 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server/authserver"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/server/status/statuspb"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils"
+	"github.com/cockroachdb/cockroach/pkg/sql/contention"
+	"github.com/cockroachdb/cockroach/pkg/sql/contentionpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/insights"
 	tablemetadatacacheutil "github.com/cockroachdb/cockroach/pkg/sql/tablemetadatacache/util"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
@@ -34,6 +37,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"github.com/cockroachdb/errors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -521,6 +525,49 @@ func TestListExecutionInsightsWhileEvictingInsights(t *testing.T) {
 	wg.Wait()
 }
 
+// TestLocalExecutionInsights tests the helper functions used by localExecutionInsights
+func TestLocalExecutionInsights(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	t.Run("validContentionInsights", func(t *testing.T) {
+		id1 := uuid.MakeV4()
+		id2 := uuid.MakeV4()
+		id3 := uuid.MakeV4()
+		insights := map[uuid.UUID]insights.Insight{
+			id1: {},
+			id2: {},
+			id3: {},
+		}
+		// id1 insight has no contention events.
+		// id2 insight has a contention event that is not resolved.
+		// id3 insight has multiple contention events, one of them is resolved.
+		events := []contentionpb.ExtendedContentionEvent{
+			{
+				WaitingTxnID: id2,
+			},
+			{
+				WaitingTxnID: id3,
+			},
+			{
+				WaitingTxnID:             id3,
+				BlockingTxnFingerprintID: 1234,
+			},
+		}
+
+		st := cluster.MakeTestingClusterSettings()
+		m := contention.NewMetrics()
+		registry := contention.NewRegistry(st, nil, &m)
+		registry.AddEventsForTest(events)
+		valid, err := validContentionInsights(registry, insights)
+		require.NoError(t, err)
+		// Only id3 should be returned.
+		require.Equal(t, 1, len(valid))
+		_, exists := valid[id3]
+		require.True(t, exists)
+	})
+}
+
 // TestStatusUpdateTableMetadataCache tests that signalling the update
 // table metadata cache job via the status server triggers the update
 // table metadata job to run.

pkg/sql/contention/event_store.go

@@ -339,6 +339,22 @@ func (s *eventStore) upsertBatch(events []contentionpb.ExtendedContentionEvent)
 	}
 }
 
+// addEventsForTest is a convenience function used by tests to directly add events to
+// the eventStore bypassing the resolver and buffer guard.
+func (s *eventStore) addEventsForTest(events []contentionpb.ExtendedContentionEvent) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	for i := range events {
+		blockingTxnID := events[i].BlockingEvent.TxnMeta.ID
+		_, ok := s.mu.store.Get(blockingTxnID)
+		if !ok {
+			atomic.AddInt64(&s.atomic.storageSize, int64(entryBytes(&events[i])))
+		}
+		s.mu.store.Add(events[i].Hash(), events[i])
+	}
+}
+
 func (s *eventStore) resolutionIntervalWithJitter() time.Duration {
 	baseInterval := TxnIDResolutionInterval.Get(&s.st.SV)
 

pkg/sql/contention/registry.go

@@ -256,6 +256,7 @@ func (r *Registry) Start(ctx context.Context, stopper *stop.Stopper) {
 func (r *Registry) AddContentionEvent(event contentionpb.ExtendedContentionEvent) {
 	r.globalLock.Lock()
 	defer r.globalLock.Unlock()
+
 	if event.ContentionType == contentionpb.ContentionType_LOCK_WAIT {
 		// (xinhaoz) We will need to change the indexMap structs if we want to surface
 		// non lock wait related contention to index contention surfaces.
@@ -288,6 +289,12 @@ func (r *Registry) AddContentionEvent(event contentionpb.ExtendedContentionEvent
 	r.eventStore.addEvent(event)
 }
 
+// AddEventsForTest is a convenience function used by tests to directly add events to
+// the underlying eventStore.
+func (r *Registry) AddEventsForTest(events []contentionpb.ExtendedContentionEvent) {
+	r.eventStore.addEventsForTest(events)
+}
+
 // ForEachEvent implements the eventReader interface.
 func (r *Registry) ForEachEvent(op func(event *contentionpb.ExtendedContentionEvent) error) error {
 	return r.eventStore.ForEachEvent(op)

pkg/sql/crdb_internal.go

@@ -776,6 +776,7 @@ CREATE TABLE crdb_internal.table_row_statistics (
 			SELECT DISTINCT ON ("tableID") "tableID", "rowCount"
 			FROM system.table_statistics
 			AS OF SYSTEM TIME '%s'
+			WHERE "partialPredicate" IS NULL
 			ORDER BY "tableID", "createdAt" DESC, "rowCount" DESC`,
 			statsAsOfTimeClusterMode.String(&p.ExecCfg().Settings.SV))
 		statRows, err := p.ExtendedEvalContext().ExecCfg.InternalDB.Executor().QueryBufferedEx(
@@ -6665,10 +6666,23 @@ CREATE VIEW crdb_internal.kv_repairable_catalog_corruptions (
 				FROM
 					system.namespace AS ns FULL JOIN system.descriptor AS d ON ns.id = d.id
 			),
+		orphaned_comments
+				AS (
+					SELECT
+						0 AS parent_id,
+						0 AS parent_schema_id,
+						'' AS name,
+						object_id AS id,
+						'comment' AS corruption
+					FROM
+						system.comments
+					WHERE
+						object_id NOT IN (SELECT id FROM system.descriptor)
+        ),
 		diag
 			AS (
 				SELECT
-					*,
+					parent_id, parent_schema_id, name, id,
 					CASE
 					WHEN descriptor IS NULL AND id != 29 THEN 'namespace'
 					WHEN updated_descriptor != repaired_descriptor THEN 'descriptor'
@@ -6677,6 +6691,8 @@ CREATE VIEW crdb_internal.kv_repairable_catalog_corruptions (
 						AS corruption
 				FROM
 					data
+				UNION
+				SELECT * FROM orphaned_comments
 			)
 	SELECT
 		parent_id, parent_schema_id, name, id, corruption

pkg/sql/faketreeeval/evalctx.go

@@ -227,6 +227,11 @@ func (ep *DummyEvalPlanner) UpsertDroppedRelationGCTTL(
 	return errors.WithStack(errEvalPlanner)
 }
 
+// UnsafeDeleteComment is part of the Planner interface.
+func (ep *DummyEvalPlanner) UnsafeDeleteComment(ctx context.Context, objectID int64) error {
+	return errors.WithStack(errEvalPlanner)
+}
+
 // UserHasAdminRole is part of the Planner interface.
 func (ep *DummyEvalPlanner) UserHasAdminRole(
 	ctx context.Context, user username.SQLUsername,

pkg/sql/logictest/testdata/logic_test/as_of

@@ -226,3 +226,12 @@ statement error pq: AS OF SYSTEM TIME: timestamp '2090-05-08 12:00:00' is in the
 CREATE VIEW wontcreate AS SELECT * FROM nonexistent AS OF SYSTEM TIME '2090-05-08 12:00:00'
 
 subtest end
+
+# Regression test for #133395. Ensure that an invalid AS OF SYSTEM TIME clause
+# requiring normalization doesn't panic and returns an error.
+statement ok
+CREATE TABLE v00 (c01 INT)
+
+statement error pq: AS OF SYSTEM TIME: expected timestamp, decimal, or interval, got bool
+SELECT ALL FROM ( v00 AS ta1401 NATURAL JOIN v00 AS ta1402 ) WITH ORDINALITY AS ta1403
+AS OF SYSTEM TIME b'any_bytes' BETWEEN SYMMETRIC 'abc' AND 'abc';

pkg/sql/logictest/testdata/logic_test/table

@@ -674,3 +674,23 @@ pr_check    CHECK ((price > 0))     true
 
 statement ok
 COMMIT;
+
+subtest end
+
+# Test that partial statistics are filtered out from table_row_statistics
+statement ok
+CREATE TABLE t (k INT PRIMARY KEY)
+
+statement ok
+INSERT INTO t SELECT generate_series(0, 9)
+
+statement ok
+ANALYZE t
+
+statement ok
+CREATE STATISTICS partial FROM t USING EXTREMES
+
+query I
+SELECT estimated_row_count FROM crdb_internal.table_row_statistics WHERE table_name = 't'
+----
+10

pkg/sql/pgwire/testdata/pgtest/procedure

@@ -67,11 +67,11 @@ until
 ReadyForQuery
 ----
 {"Type":"RowDescription","Fields":null}
-{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"foo","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func378","UnknownFields":null}
+{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"foo","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func379","UnknownFields":null}
 {"Type":"RowDescription","Fields":null}
-{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"bar","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func378","UnknownFields":null}
+{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"bar","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func379","UnknownFields":null}
 {"Type":"RowDescription","Fields":null}
-{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"baz","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func378","UnknownFields":null}
+{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"baz","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func379","UnknownFields":null}
 {"Type":"CommandComplete","CommandTag":"CALL"}
 {"Type":"ReadyForQuery","TxStatus":"I"}
 
@@ -87,10 +87,10 @@ ReadyForQuery
 ----
 {"Type":"ParseComplete"}
 {"Type":"BindComplete"}
-{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"foo","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func378","UnknownFields":null}
+{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"foo","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func379","UnknownFields":null}
 {"Type":"RowDescription","Fields":null}
-{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"bar","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func378","UnknownFields":null}
+{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"bar","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func379","UnknownFields":null}
 {"Type":"RowDescription","Fields":null}
-{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"baz","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func378","UnknownFields":null}
+{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"baz","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func379","UnknownFields":null}
 {"Type":"CommandComplete","CommandTag":"CALL"}
 {"Type":"ReadyForQuery","TxStatus":"I"}