Merge #28143 #28217 #28219

28143: sql: fix the handling of tuples in distsql r=knz a=knz Fixes #26624. Supersedes #28100. This patch fixes the bug where 0-valued and 1-valued tuples were not properly serialized in distsql processor/flow specs, causing these special tuples to not be properly supported for distributed execution. It also fixes the bug where labeled DTuples were not serialized with their tuples, causing labeled tuples to be broken with distributed execution. Release note: None 28217: storage: dissallow empty HeartbeatRequest.Now r=andreimatei a=andreimatei Release note: None 28219: distsql: fix a missing context in log message r=arjunravinarayan a=arjunravinarayan Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: Andrei Matei <[email protected]> Co-authored-by: Arjun Narayan <[email protected]>
cockroachdb · Aug 2, 2018 · 936b997 · 936b997
4 parents c754381 + c35d8e4 + 0805f4d + d1ab0f7
commit 936b997
Show file tree

Hide file tree

Showing 30 changed files with 546 additions and 783 deletions.
diff --git a/docs/generated/sql/bnf/on_conflict.bnf b/docs/generated/sql/bnf/on_conflict.bnf
@@ -1,3 +1,3 @@
 on_conflict ::=
-	'ON' 'CONFLICT' ( '(' ( ( name ) ( ( ',' name ) )* ) ')' ( 'WHERE' a_expr |  ) |  ) 'DO' 'UPDATE' 'SET' ( ( ( ( column_name '=' a_expr ) | ( '(' ( ( ( column_name ) ) ( ( ',' ( column_name ) ) )* ) ')' '=' ( '(' select_stmt ')' | '(' ( ( a_expr ) ( ( ',' a_expr ) )* ) ')' ) ) ) ) ( ( ',' ( ( column_name '=' a_expr ) | ( '(' ( ( ( column_name ) ) ( ( ',' ( column_name ) ) )* ) ')' '=' ( '(' select_stmt ')' | '(' ( ( a_expr ) ( ( ',' a_expr ) )* ) ')' ) ) ) ) )* ) ( 'WHERE' a_expr |  )
+	'ON' 'CONFLICT' ( '(' ( ( name ) ( ( ',' name ) )* ) ')' ( 'WHERE' a_expr |  ) |  ) 'DO' 'UPDATE' 'SET' ( ( ( ( column_name '=' a_expr ) | ( '(' ( ( ( column_name ) ) ( ( ',' ( column_name ) ) )* ) ')' '=' ( '(' select_stmt ')' | ( '(' ')' | '(' ( a_expr | a_expr ',' | a_expr ',' ( ( a_expr ) ( ( ',' a_expr ) )* ) ) ')' ) ) ) ) ) ( ( ',' ( ( column_name '=' a_expr ) | ( '(' ( ( ( column_name ) ) ( ( ',' ( column_name ) ) )* ) ')' '=' ( '(' select_stmt ')' | ( '(' ')' | '(' ( a_expr | a_expr ',' | a_expr ',' ( ( a_expr ) ( ( ',' a_expr ) )* ) ) ')' ) ) ) ) ) )* ) ( 'WHERE' a_expr |  )
 	| 'ON' 'CONFLICT' ( '(' ( ( name ) ( ( ',' name ) )* ) ')' ( 'WHERE' a_expr |  ) |  ) 'DO' 'NOTHING'
diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf
@@ -989,7 +989,7 @@ b_expr ::=
 
 in_expr ::=
 	select_with_parens
-	| '(' expr_list ')'
+	| expr_tuple1_ambiguous
 
 subquery_op ::=
 	math_op
@@ -1305,8 +1305,7 @@ d_expr ::=
 	| select_with_parens
 	| 'ARRAY' select_with_parens
 	| 'ARRAY' array_expr
-	| row
-	| '(' row 'AS' name_list ')'
+	| labeled_row
 
 array_subscripts ::=
 	( array_subscript ) ( ( array_subscript ) )*
@@ -1331,6 +1330,10 @@ postgres_oid ::=
 	| 'REGTYPE'
 	| 'REGNAMESPACE'
 
+expr_tuple1_ambiguous ::=
+	'(' ')'
+	| '(' tuple1_ambiguous_values ')'
+
 math_op ::=
 	'+'
 	| '-'
@@ -1612,9 +1615,9 @@ array_expr ::=
 	'[' opt_expr_list ']'
 	| '[' array_expr_list ']'
 
-row ::=
-	'ROW' '(' opt_expr_list ')'
-	| '(' expr_list ',' a_expr ')'
+labeled_row ::=
+	row
+	| '(' row 'AS' name_list ')'
 
 array_subscript ::=
 	'[' a_expr ']'
@@ -1656,6 +1659,11 @@ opt_interval ::=
 	| 'MINUTE' 'TO' interval_second
 	| 
 
+tuple1_ambiguous_values ::=
+	a_expr
+	| a_expr ','
+	| a_expr ',' expr_list
+
 scrub_option ::=
 	'INDEX' 'ALL'
 	| 'INDEX' '(' name_list ')'
@@ -1865,6 +1873,10 @@ opt_expr_list ::=
 array_expr_list ::=
 	( array_expr ) ( ( ',' array_expr ) )*
 
+row ::=
+	'ROW' '(' opt_expr_list ')'
+	| expr_tuple_unambiguous
+
 opt_slice_bound ::=
 	a_expr
 	| 
@@ -2027,6 +2039,10 @@ special_function ::=
 	| 'GREATEST' '(' expr_list ')'
 	| 'LEAST' '(' expr_list ')'
 
+expr_tuple_unambiguous ::=
+	'(' ')'
+	| '(' tuple1_unambiguous_values ')'
+
 window_definition ::=
 	window_name 'AS' window_specification
 
@@ -2107,6 +2123,10 @@ trim_list ::=
 	| 'FROM' expr_list
 	| expr_list
 
+tuple1_unambiguous_values ::=
+	a_expr ','
+	| a_expr ',' expr_list
+
 index_hints_param ::=
 	'FORCE_INDEX' '=' index_name
 	| 'NO_INDEX_JOIN'

diff --git a/docs/generated/sql/bnf/update_stmt.bnf b/docs/generated/sql/bnf/update_stmt.bnf
@@ -1,2 +1,2 @@
 update_stmt ::=
-	( ( 'WITH' ( ( common_table_expr ) ( ( ',' common_table_expr ) )* ) ) |  ) 'UPDATE' ( table_name | table_name table_alias_name | table_name 'AS' table_alias_name ) 'SET' ( ( ( ( column_name '=' a_expr ) | ( '(' ( ( ( column_name ) ) ( ( ',' ( column_name ) ) )* ) ')' '=' ( '(' select_stmt ')' | '(' ( ( a_expr ) ( ( ',' a_expr ) )* ) ')' ) ) ) ) ( ( ',' ( ( column_name '=' a_expr ) | ( '(' ( ( ( column_name ) ) ( ( ',' ( column_name ) ) )* ) ')' '=' ( '(' select_stmt ')' | '(' ( ( a_expr ) ( ( ',' a_expr ) )* ) ')' ) ) ) ) )* ) ( 'WHERE' a_expr |  ) ( sort_clause |  ) ( limit_clause |  ) ( 'RETURNING' target_list | 'RETURNING' 'NOTHING' |  )
+	( ( 'WITH' ( ( common_table_expr ) ( ( ',' common_table_expr ) )* ) ) |  ) 'UPDATE' ( table_name | table_name table_alias_name | table_name 'AS' table_alias_name ) 'SET' ( ( ( ( column_name '=' a_expr ) | ( '(' ( ( ( column_name ) ) ( ( ',' ( column_name ) ) )* ) ')' '=' ( '(' select_stmt ')' | ( '(' ')' | '(' ( a_expr | a_expr ',' | a_expr ',' ( ( a_expr ) ( ( ',' a_expr ) )* ) ) ')' ) ) ) ) ) ( ( ',' ( ( column_name '=' a_expr ) | ( '(' ( ( ( column_name ) ) ( ( ',' ( column_name ) ) )* ) ')' '=' ( '(' select_stmt ')' | ( '(' ')' | '(' ( a_expr | a_expr ',' | a_expr ',' ( ( a_expr ) ( ( ',' a_expr ) )* ) ) ')' ) ) ) ) ) )* ) ( 'WHERE' a_expr |  ) ( sort_clause |  ) ( limit_clause |  ) ( 'RETURNING' target_list | 'RETURNING' 'NOTHING' |  )
diff --git a/pkg/ccl/partitionccl/partition_test.go b/pkg/ccl/partitionccl/partition_test.go
@@ -1191,10 +1191,10 @@ func TestSelectPartitionExprs(t *testing.T) {
 		{`p33dp44d`, `(((a, b) = (3, 3)) AND (NOT ((a, b, c) = (3, 3, 5)))) OR (((a, b) = (4, 4)) AND (NOT ((a, b, c) = (4, 4, 5))))`},
 		// NB See the TODO in the impl for why this next case has some clearly
 		// unrelated `!=`s.
-		{`p6d`, `((a) = (6)) AND (NOT (((a, b) = (3, 3)) OR ((a, b) = (4, 4))))`},
-		{`pdd`, `NOT ((((a, b) = (3, 3)) OR ((a, b) = (4, 4))) OR ((a) = (6)))`},
+		{`p6d`, `((a,) = (6,)) AND (NOT (((a, b) = (3, 3)) OR ((a, b) = (4, 4))))`},
+		{`pdd`, `NOT ((((a, b) = (3, 3)) OR ((a, b) = (4, 4))) OR ((a,) = (6,)))`},
 
-		{`p335p445,p6d`, `(((a, b, c) = (3, 3, 5)) OR ((a, b, c) = (4, 4, 5))) OR (((a) = (6)) AND (NOT (((a, b) = (3, 3)) OR ((a, b) = (4, 4)))))`},
+		{`p335p445,p6d`, `(((a, b, c) = (3, 3, 5)) OR ((a, b, c) = (4, 4, 5))) OR (((a,) = (6,)) AND (NOT (((a, b) = (3, 3)) OR ((a, b) = (4, 4)))))`},
 
 		// TODO(dan): The expression simplification in this method is all done
 		// by our normal SQL expression simplification code. Seems like it could

diff --git a/pkg/cmd/docgen/diagrams.go b/pkg/cmd/docgen/diagrams.go
@@ -706,8 +706,10 @@ var specs = []stmtSpec{
 		nosplit: true,
 	},
 	{
-		name:   "on_conflict",
-		inline: []string{"opt_conf_expr", "name_list", "where_clause", "set_clause_list", "insert_column_list", "insert_column_item", "set_clause", "single_set_clause", "multiple_set_clause", "in_expr", "expr_list"},
+		name: "on_conflict",
+		inline: []string{"opt_conf_expr", "name_list", "where_clause", "set_clause_list", "insert_column_list",
+			"insert_column_item", "set_clause", "single_set_clause", "multiple_set_clause", "in_expr", "expr_list",
+			"expr_tuple1_ambiguous", "tuple1_ambiguous_values"},
 		replace: map[string]string{
 			"select_with_parens": "'(' select_stmt ')'",
 		},
@@ -1088,6 +1090,8 @@ var specs = []stmtSpec{
 			"multiple_set_clause",
 			"in_expr",
 			"expr_list",
+			"expr_tuple1_ambiguous",
+			"tuple1_ambiguous_values",
 			"where_clause",
 			"opt_sort_clause",
 			"returning_clause",

diff --git a/pkg/kv/txn_coord_sender_test.go b/pkg/kv/txn_coord_sender_test.go
@@ -404,20 +404,25 @@ func TestTxnCoordSenderHeartbeat(t *testing.T) {
 
 // getTxn fetches the requested key and returns the transaction info.
 func getTxn(ctx context.Context, txn *client.Txn) (*roachpb.Transaction, *roachpb.Error) {
-	hb := &roachpb.HeartbeatTxnRequest{
+	txnMeta := txn.Proto().TxnMeta
+	qt := &roachpb.QueryTxnRequest{
 		RequestHeader: roachpb.RequestHeader{
-			Key: txn.Proto().Key,
+			Key: txnMeta.Key,
 		},
+		Txn: txnMeta,
 	}
+
 	ba := roachpb.BatchRequest{}
-	ba.Header = roachpb.Header{Txn: txn.Proto()}
-	ba.Add(hb)
+	ba.Add(qt)
+
+	db := txn.DB()
+	sender := db.NonTransactionalSender()
 
-	br, pErr := txn.Send(ctx, ba)
+	br, pErr := sender.Send(ctx, ba)
 	if pErr != nil {
 		return nil, pErr
 	}
-	return br.Txn, nil
+	return &br.Responses[0].GetInner().(*roachpb.QueryTxnResponse).QueriedTxn, nil
 }
 
 func verifyCleanup(key roachpb.Key, eng engine.Engine, t *testing.T, coords ...*TxnCoordSender) {

diff --git a/pkg/sql/distsqlplan/expression.go b/pkg/sql/distsqlplan/expression.go
@@ -19,7 +19,6 @@ package distsqlplan
 
 import (
 	"bytes"
-	"context"
 	"fmt"
 
 	"github.com/cockroachdb/cockroach/pkg/sql/distsqlrun"
@@ -74,7 +73,7 @@ func MakeExpression(
 	}
 	fmtCtx.FormatNode(expr)
 	if log.V(1) {
-		log.Infof(context.TODO(), "Expr %s:\n%s", buf.String(), tree.ExprDebugString(expr))
+		log.Infof(evalCtx.Ctx(), "Expr %s:\n%s", buf.String(), tree.ExprDebugString(expr))
 	}
 	return distsqlrun.Expression{Expr: buf.String()}
 }
diff --git a/pkg/sql/logictest/testdata/logic_test/statement_statistics b/pkg/sql/logictest/testdata/logic_test/statement_statistics
@@ -108,7 +108,7 @@ SELECT key,flags
 ----
 key                                                               flags
 INSERT INTO test VALUES (_, _, __more1__), (__more1__)            -
-SELECT ROW(_, _, __more3__) FROM test WHERE _                     ·
+SELECT (_, _, __more3__) FROM test WHERE _                        ·
 SELECT key FROM test.crdb_internal.node_statement_statistics      ·
 SELECT sin(_)                                                     ·
 SELECT sqrt(-_)                                                   !
@@ -132,7 +132,7 @@ SELECT anonymized
  WHERE application_name = 'valuetest' ORDER BY key
 ----
 INSERT INTO _ VALUES (_, _, __more1__), (__more1__)
-SELECT ROW(_, _, __more3__) FROM _ WHERE _
+SELECT (_, _, __more3__) FROM _ WHERE _
 SELECT _ FROM _.crdb_internal.node_statement_statistics
 SELECT sin(_)
 SELECT sqrt(-_)