Merge cockroachdb#95398 cockroachdb#95414 cockroachdb#95426

95398: sql/catalog/schemaexpr: fix bug when new column default has different… r=ajwerner a=ajwerner

… type

Since 22.2 we permit default expressions to contain types which are not exactly the same as the column type; it is valid to have an expression which can be cast to the column type in an assignment context. Generally, the optimizer handles inserting the assignment cast into the execution of the relevant mutations. Unfortunately, the cast was not present for backfills.

This PR detects the situation where a cast is needed and insert it directly into the plan of the backfill (or import).

Epic: None

Fixes: cockroachdb#93398

Release note (bug fix): Since 22.2, default expressions can have a type which differs from the type of the column so long as the expression's type can be cast in an assignment context. Unfortunately, this code had a bug when adding new columns. The code in the backfill logic was not sophisticated enough to know to add the cast; when such a default expression was added to a new column it would result in a panic during the backfill. This bug has now been fixed.

95414: sql: fix CLOSE ALL so it doesn't ignore the ALL flag r=jordanlewis a=rafiss

informs cockroachdb#83061

Release note (bug fix): Fixed a bug where CLOSE ALL would not respect the "ALL" flag and would instead attempt to close a cursor with no name.

95426: gossip: don't resolve addresses while holding mutex r=erikgrinaker a=erikgrinaker

This patch removes a DNS resolution call performed while holding the gossip mutex. This can lead to severe process stalls if the DNS lookup is not immediate, since we need to acquire gossip read locks in several performance critical code paths, including Raft processing. However, the DNS lookup was only done when validating a remote forwarding address, which presumably happens fairly rarely. Removing it should not cause any problems, since the address will necessarily be validated later when attempting to connect to it.

Epic: none
Release note (bug fix): Fixed a bug where a DNS lookup was performed during gossip remote forwarding while holding the gossip mutex. This could cause processing stalls if the DNS server was slow to respond.

Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>

pkg/gossip/client.go

@@ -259,13 +259,6 @@ func (c *client) handleResponse(ctx context.Context, g *Gossip, reply *Response)
 				"received forward from n%d to n%d (%s); already have active connection, skipping",
 				reply.NodeID, reply.AlternateNodeID, reply.AlternateAddr)
 		}
-		// We try to resolve the address, but don't actually use the result.
-		// The certificates (if any) may only be valid for the unresolved
-		// address.
-		if _, err := reply.AlternateAddr.Resolve(); err != nil {
-			return errors.Wrapf(err, "unable to resolve alternate address %s for n%d",
-				reply.AlternateAddr, reply.AlternateNodeID)
-		}
 		c.forwardAddr = reply.AlternateAddr
 		return errors.Errorf("received forward from n%d to n%d (%s)",
 			reply.NodeID, reply.AlternateNodeID, reply.AlternateAddr)

pkg/sql/catalog/schemaexpr/default_exprs.go

@@ -18,6 +18,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/transform"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/errors"
 )
 
 // MakeDefaultExprs returns a slice of the default expressions for the slice
@@ -71,6 +72,20 @@ func MakeDefaultExprs(
 		if err != nil {
 			return nil, err
 		}
+		// For "reasons" the CastExpr type check ignores the desired type. That
+		// means that we can get a result here that is not the right type, and
+		// we'd need to wrap that in an explicit cast. This is the equivalent of
+		// an assignment cast we'd insert when writing to the table directly.
+		if !typedExpr.ResolvedType().Equivalent(col.GetType()) {
+			if typedExpr, err = tree.TypeCheck(ctx, &tree.CastExpr{
+				Expr:       typedExpr,
+				Type:       col.GetType(),
+				SyntaxMode: tree.CastExplicit,
+			}, semaCtx, col.GetType()); err != nil {
+				return nil, errors.NewAssertionErrorWithWrappedErrf(err,
+					"failed to type check the cast of %v to %v", expr, col.GetType())
+			}
+		}
 		if typedExpr, err = txCtx.NormalizeExpr(ctx, evalCtx, typedExpr); err != nil {
 			return nil, err
 		}

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -2751,3 +2751,22 @@ SELECT count(*) from t_with_dropped_index_expr;
 
 statement ok
 RESUME JOB (SELECT job_id FROM crdb_internal.jobs WHERE description LIKE 'ALTER TABLE %t_with_dropped_index_expr DROP COLUMN j CASCADE%' AND status='paused' FETCH FIRST 1 ROWS ONLY);
+
+
+# In 23.1 we added support for default expressions which can be assignment cast
+# to the column type. This work introduced a bug whereby the backfill logic
+# would not apply the appropriate cast. This test ensures that such tables can
+# be backfilled.
+subtest add_column_with_default_expression_with_different_type
+
+statement ok
+CREATE TABLE t_93398 (c1 INT);
+INSERT INTO t_93398 VALUES (0);
+
+statement ok
+ALTER TABLE t_93398 ADD COLUMN c2 DECIMAL DEFAULT pi();
+
+query IT
+SELECT * FROM t_93398;
+----
+0  3.141592653589793

pkg/sql/logictest/testdata/logic_test/cursor

@@ -1,3 +1,6 @@
+statement ok
+CLOSE ALL
+
 statement ok
 CREATE TABLE a (a INT PRIMARY KEY, b INT);
 INSERT INTO a VALUES (1, 2), (2, 3)
@@ -46,6 +49,38 @@ CLOSE foo
 
 statement ok
 COMMIT;
+BEGIN;
+DECLARE foo CURSOR FOR SELECT * FROM a ORDER BY a
+
+query II
+FETCH 1 foo
+----
+1  2
+
+statement ok
+CLOSE foo
+
+statement error cursor \"foo\" does not exist
+FETCH 2 foo
+
+statement ok
+ROLLBACK;
+BEGIN;
+DECLARE foo CURSOR FOR SELECT * FROM a ORDER BY a
+
+query II
+FETCH 1 foo
+----
+1  2
+
+statement ok
+CLOSE ALL
+
+statement error cursor \"foo\" does not exist
+FETCH 2 foo
+
+statement ok
+ROLLBACK;
 
 statement error cursor \"foo\" does not exist
 BEGIN;

pkg/sql/sql_cursor.go

@@ -243,6 +243,9 @@ func (p *planner) CloseCursor(ctx context.Context, n *tree.CloseCursor) (planNod
 	return &delayedNode{
 		name: n.String(),
 		constructor: func(ctx context.Context, p *planner) (planNode, error) {
+			if n.All {
+				return newZeroNode(nil /* columns */), p.sqlCursors.closeAll(false /* errorOnWithHold */)
+			}
 			return newZeroNode(nil /* columns */), p.sqlCursors.closeCursor(n.Name)
 		},
 	}, nil