Merge #156813 #156819 #156879

156813: sql: wrap CASE expressions with parenthesis within PLpgSQL context r=yuzefovich a=yuzefovich

Currently, if we generate a CASE expression within the condition of the IF block in PLpgSQL, it cannot be parsed because it's unclear whether THEN belongs to CASE or to IF. In order to disambiguate this, we need to wrap CASE expressions in parenthesis, and we now do so in all sqlsmith-generated queries whenever we're in PLpgSQL context.

Fixes: #155200.
Release note: None

156819: CODEOWNERS: remove sql-syntax-prs team r=yuzefovich a=yuzefovich

Divide ownership between Queries and Foundations.

Epic: None
Release note: None

156879: opt: avoid more types of bad generic query plans r=mgartner a=mgartner

This commit extends #155163. Instead of tracking an auxiliary boolean
value to indicate that a plan has one or more expressions with unbounded
cardinality, the `memo.Cost` struct now tracks the number of read
expressions (i.e., expressions that perform KV reads) that have
unbounded cardinality. This helps to avoid picking generic query plans
that will perform significantly worse than their related custom query
plans.

Fixes #156690

Release note: None


Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>

Author: 3 people

.github/CODEOWNERS

@@ -71,8 +71,11 @@
 /pkg/sql/executor_statement_metrics.go @cockroachdb/obs-prs
 /pkg/sql/flowinfra/          @cockroachdb/sql-queries-prs
 /pkg/sql/hints/              @cockroachdb/sql-queries-prs
+/pkg/sql/parser/             @cockroachdb/sql-queries-prs
 /pkg/sql/physicalplan/       @cockroachdb/sql-queries-prs
 /pkg/sql/row*                @cockroachdb/sql-queries-prs
+/pkg/sql/sem/tree/           @cockroachdb/sql-queries-prs
+/pkg/sql/types/              @cockroachdb/sql-queries-prs
 /pkg/sql/control_job*        @cockroachdb/sql-queries-prs @cockroachdb/jobs-prs
 /pkg/sql/job_exec_context*   @cockroachdb/sql-queries-prs @cockroachdb/jobs-prs
 /pkg/sql/delegate/*job*.go   @cockroachdb/jobs-prs @cockroachdb/disaster-recovery
@@ -92,14 +95,10 @@
 /pkg/sql/tablemetadatacache/  @cockroachdb/obs-prs
 /pkg/ccl/testccl/sqlstatsccl/ @cockroachdb/obs-prs
 
-/pkg/sql/sem/tree/           @cockroachdb/sql-syntax-prs
-/pkg/sql/parser/             @cockroachdb/sql-syntax-prs
-/pkg/sql/lex/                @cockroachdb/sql-syntax-prs
-/pkg/sql/show_create*.go     @cockroachdb/sql-syntax-prs
-/pkg/sql/types/              @cockroachdb/sql-syntax-prs
-
 /pkg/sql/crdb_internal.go    @cockroachdb/sql-foundations
 /pkg/sql/pg_catalog.go       @cockroachdb/sql-foundations
+/pkg/sql/show_create*.go     @cockroachdb/sql-foundations
+/pkg/sql/lex/                @cockroachdb/sql-foundations
 /pkg/sql/pgwire/             @cockroachdb/sql-foundations @cockroachdb/product-security
 /pkg/sql/pgwire/auth.go      @cockroachdb/sql-foundations @cockroachdb/security-engineering
 /pkg/sql/sem/builtins/       @cockroachdb/sql-foundations

TEAMS.yaml

@@ -7,7 +7,6 @@ cockroachdb/docs:
     cockroachdb/docs-infra-prs: other
 cockroachdb/sql-foundations:
   aliases:
-    cockroachdb/sql-syntax-prs: other
     cockroachdb/sqlproxy-prs: other
     cockroachdb/sql-api-prs: other
   label: T-sql-foundations

pkg/internal/sqlsmith/plpgsql.go

@@ -14,6 +14,8 @@ import (
 	"github.com/cockroachdb/errors"
 )
 
+var plpgsqlFlags = tree.FmtParsable | tree.FmtPLpgSQLParen
+
 func (s *Smither) makeRoutineBodyPLpgSQL(
 	params tree.ParamTypes, rTyp *types.T, vol tree.RoutineVolatility,
 ) string {
@@ -25,7 +27,7 @@ func (s *Smither) makeRoutineBodyPLpgSQL(
 	// errors.
 	block := s.makePLpgSQLBlock(scope)
 	block.Body = append(block.Body, s.makePLpgSQLReturn(scope))
-	return "\n" + tree.AsStringWithFlags(s.makePLpgSQLBlock(scope), tree.FmtParsable)
+	return "\n" + tree.AsStringWithFlags(s.makePLpgSQLBlock(scope), plpgsqlFlags, tree.FmtInPLpgSQL(true /* inPLpgSQL */))
 }
 
 func (s *Smither) makePLpgSQLBlock(scope plpgsqlBlockScope) *ast.Block {

pkg/internal/sqlsmith/setup_test.go

@@ -8,8 +8,6 @@ package sqlsmith_test
 import (
 	"context"
 	"flag"
-	"fmt"
-	"strings"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
@@ -92,9 +90,7 @@ func TestGenerateParse(t *testing.T) {
 		t.Fatalf("unknown setting %s", settingName)
 	}
 	settings := setting(rnd)
-	t.Log("setting:", settingName, settings.Options)
 	setupSQL := setup(rnd)
-	t.Log(strings.Join(setupSQL, "\n"))
 	for _, stmt := range setupSQL {
 		db.Exec(t, stmt)
 	}
@@ -119,7 +115,6 @@ func TestGenerateParse(t *testing.T) {
 		if err != nil {
 			t.Fatal(err)
 		}
-		fmt.Print("STMT: ", i, "\n", stmt, ";\n\n")
 		if *flagExec {
 			_, err = conn.ExecContext(ctx, `SET statement_timeout = '9s'`)
 			if err != nil {
@@ -129,7 +124,7 @@ func TestGenerateParse(t *testing.T) {
 				es := err.Error()
 				if !seen[es] {
 					seen[es] = true
-					fmt.Printf("ERR (%d): %v\n", i, err)
+					t.Logf("ERR (%d): %v\n", i, err)
 				}
 			}
 		}

pkg/internal/sqlsmith/sqlsmith.go

@@ -192,6 +192,7 @@ var prettyCfg = func() tree.PrettyCfg {
 	cfg := tree.DefaultPrettyCfg()
 	cfg.LineWidth = 120
 	cfg.Simplify = false
+	cfg.FmtFlags = tree.FmtPLpgSQLParen
 	return cfg
 }()
 
@@ -213,10 +214,10 @@ func (s *Smither) Generate() string {
 		i = 0
 
 		printCfg := prettyCfg
-		fl := tree.FmtParsable
+		fl := plpgsqlFlags
 		if s.postgres {
-			printCfg.FmtFlags = tree.FmtPGCatalog
-			fl = tree.FmtPGCatalog
+			printCfg.FmtFlags = tree.FmtPGCatalog | tree.FmtPLpgSQLParen
+			fl = tree.FmtPGCatalog | tree.FmtPLpgSQLParen
 		}
 		p, err := printCfg.Pretty(stmt)
 		if err != nil {

pkg/internal/team/TEAMS.yaml

@@ -7,7 +7,6 @@ cockroachdb/docs:
     cockroachdb/docs-infra-prs: other
 cockroachdb/sql-foundations:
   aliases:
-    cockroachdb/sql-syntax-prs: other
     cockroachdb/sqlproxy-prs: other
     cockroachdb/sql-api-prs: other
   label: T-sql-foundations

pkg/sql/logictest/testdata/logic_test/generic

@@ -474,8 +474,9 @@ plan type: custom
 statement ok
 DEALLOCATE p
 
-# Regression test for #155159. Do not choose a generic query plan with unbounded
-# cardinality when the custom plans have bounded cardinality.
+# Regression test for #155159 and #156690. Do not choose a generic query plan
+# with reads that have unbounded cardinality when the reads in the custom plan
+# have bounded cardinality.
 statement ok
 CREATE TABLE t155159 (
   id INT PRIMARY KEY,
@@ -540,3 +541,84 @@ quality of service: regular
 
 statement ok
 DEALLOCATE p
+
+statement ok
+PREPARE p AS
+SELECT * FROM (
+  SELECT id FROM t155159 WHERE a = $1 AND b >= $2
+  ORDER BY b, id
+  LIMIT 250
+)
+UNION
+SELECT * FROM (
+  SELECT id FROM t155159 WHERE a = $3 AND b >= $4
+)
+
+statement ok
+EXECUTE p (33, 44, 55, 66)
+
+statement ok
+EXECUTE p (33, 44, 55, 66)
+
+statement ok
+EXECUTE p (33, 44, 55, 66)
+
+statement ok
+EXECUTE p (33, 44, 55, 66)
+
+statement ok
+EXECUTE p (33, 44, 55, 66)
+
+query T
+EXPLAIN ANALYZE EXECUTE p (33, 44, 55, 66);
+----
+planning time: 10µs
+execution time: 100µs
+distribution: <hidden>
+vectorized: <hidden>
+plan type: custom
+maximum memory usage: <hidden>
+DistSQL network usage: <hidden>
+regions: <hidden>
+isolation level: serializable
+priority: normal
+quality of service: regular
+·
+• union
+│ sql nodes: <hidden>
+│ regions: <hidden>
+│ actual row count: 0
+│ execution time: 0µs
+│ estimated max memory allocated: 0 B
+│
+├── • scan
+│     sql nodes: <hidden>
+│     kv nodes: <hidden>
+│     regions: <hidden>
+│     actual row count: 0
+│     KV time: 0µs
+│     KV rows decoded: 0
+│     KV bytes read: 0 B
+│     KV gRPC calls: 0
+│     estimated max memory allocated: 0 B
+│     missing stats
+│     table: t155159@t155159_a_b_idx
+│     spans: [/33/44 - /33]
+│     limit: 250
+│
+└── • scan
+      sql nodes: <hidden>
+      kv nodes: <hidden>
+      regions: <hidden>
+      actual row count: 0
+      KV time: 0µs
+      KV rows decoded: 0
+      KV bytes read: 0 B
+      KV gRPC calls: 0
+      estimated max memory allocated: 0 B
+      missing stats
+      table: t155159@t155159_a_b_idx
+      spans: [/55/66 - /55]
+
+statement ok
+DEALLOCATE p

pkg/sql/opt/memo/cost.go

@@ -22,14 +22,12 @@ type Cost struct {
 	// cost is compared to other costs with Less.
 	aux struct {
 		// fullScanCount is the number of full table or index scans in a
-		// sub-plan, up to 255.
-		fullScanCount uint8
-		// unboundedCardinality is true if the operator or any of its
-		// descendants have no guaranteed upperbound on the number of rows that
-		// they can produce. It is similar to UnboundedCardinalityPenalty, but
-		// different in that it is used to propagate the same information up the
-		// tree without affecting cost comparisons.
-		unboundedCardinality bool
+		// sub-plan, up to 65535.
+		fullScanCount uint16
+		// unboundedReadCount is the number of read expressions (e.g., scans,
+		// lookup joins, etc.) in a sub-plan that have no upper-bound
+		// cardinality, up to 65535.
+		unboundedReadCount uint16
 	}
 }
 
@@ -63,45 +61,28 @@ func (c Cost) Less(other Cost) bool {
 func (c *Cost) Add(other Cost) {
 	c.C += other.C
 	c.Penalties |= other.Penalties
-	if c.aux.fullScanCount > math.MaxUint8-other.aux.fullScanCount {
-		// Avoid overflow.
-		c.aux.fullScanCount = math.MaxUint8
-	} else {
-		c.aux.fullScanCount += other.aux.fullScanCount
-	}
-	c.aux.unboundedCardinality = c.aux.unboundedCardinality || other.aux.unboundedCardinality
+	c.aux.fullScanCount = addUint16(c.aux.fullScanCount, other.aux.fullScanCount)
+	c.aux.unboundedReadCount = addUint16(c.aux.unboundedReadCount, other.aux.unboundedReadCount)
 }
 
 // FullScanCount returns the number of full scans in the cost.
-func (c Cost) FullScanCount() uint8 {
+func (c Cost) FullScanCount() uint16 {
 	return c.aux.fullScanCount
 }
 
 // IncrFullScanCount increments that auxiliary full scan count within c.
 func (c *Cost) IncrFullScanCount() {
-	// Avoid overflow.
-	if c.aux.fullScanCount < math.MaxUint8 {
-		c.aux.fullScanCount++
-	}
+	c.aux.fullScanCount = addUint16(c.aux.fullScanCount, 1)
 }
 
-// HasUnboundedCardinality returns true if any expression in the tree has no
-// guaranteed upperbound on the number of rows that it will produce.
-//
-// NOTE: The returned value is independent of the UnboundedCardinalityPenalty
-// and true may be returned when the penalty is not set. It has no effect on
-// cost comparisons.
-func (c Cost) HasUnboundedCardinality() bool {
-	return c.aux.unboundedCardinality
+// UnboundedReadCount returns the number of full scans in the cost.
+func (c Cost) UnboundedReadCount() uint16 {
+	return c.aux.unboundedReadCount
 }
 
-// SetUnboundedCardinality is called to indicate that an expression has no
-// guaranteed upperbound on the number of rows that it will produce.
-//
-// NOTE: This flag does not affect cost comparisons and is independent of the
-// UnboundedCardinalityPenalty.
-func (c *Cost) SetUnboundedCardinality() {
-	c.aux.unboundedCardinality = true
+// IncrUnboundedReadCount increments that auxiliary full scan count within c.
+func (c *Cost) IncrUnboundedReadCount() {
+	c.aux.unboundedReadCount = addUint16(c.aux.unboundedReadCount, 1)
 }
 
 // Penalties is an ordered bitmask where each bit indicates a cost penalty. The
@@ -142,12 +123,11 @@ const (
 //	<Cost> is the floating point cost value.
 //	<Penalties> contains "H", "F", or "U" for HugeCostPenalty, FullScanPenalty,
 //	  and UnboundedCardinalityPenalty, respectively.
-//	<aux> contains a number for full scan count and "u" for
-//	  unboundedCardinality.
+//	<aux> contains the number of full scans and unbounded reads.
 //
-// For example, the summary "1.23:HF:5fu" indicates a cost of 1.23 with the
-// HugeCostPenalty and FullScanPenalty penalties, 5 full scans, and the
-// unboundedCardinality flag set.
+// For example, the summary "1.23:HF:5f6u" indicates a cost of 1.23 with the
+// HugeCostPenalty and FullScanPenalty penalties, 5 full scans, and 6 unbounded
+// reads.
 func (c Cost) Summary() string {
 	var sb strings.Builder
 	_, _ = fmt.Fprintf(&sb, "%.9g:", c.C)
@@ -161,8 +141,13 @@ func (c Cost) Summary() string {
 		sb.WriteByte('U')
 	}
 	_, _ = fmt.Fprintf(&sb, ":%df", c.aux.fullScanCount)
-	if c.aux.unboundedCardinality {
-		sb.WriteByte('u')
-	}
+	_, _ = fmt.Fprintf(&sb, "%du", c.aux.unboundedReadCount)
 	return sb.String()
 }
+
+func addUint16(a, b uint16) uint16 {
+	if a > math.MaxUint16-b {
+		return math.MaxUint16
+	}
+	return a + b
+}

pkg/sql/opt/memo/cost_test.go

@@ -8,8 +8,8 @@ package memo
 import "testing"
 
 type testAux struct {
-	fullScanCount        uint8
-	unboundedCardinality bool
+	fullScanCount      uint16
+	unboundedReadCount uint16
 }
 
 func TestCostLess(t *testing.T) {
@@ -39,8 +39,8 @@ func TestCostLess(t *testing.T) {
 		{Cost{C: 2.0}, Cost{C: 1.0, Penalties: UnboundedCardinalityPenalty}, true},
 		{Cost{C: 1.0, Penalties: UnboundedCardinalityPenalty}, Cost{C: 2.0}, false},
 		// Auxiliary information should not affect the comparison.
-		{Cost{C: 1.0, aux: testAux{0, false}}, Cost{C: 1.0, aux: testAux{1, true}}, false},
-		{Cost{C: 1.0, aux: testAux{1, true}}, Cost{C: 1.0, aux: testAux{0, false}}, false},
+		{Cost{C: 1.0, aux: testAux{0, 0}}, Cost{C: 1.0, aux: testAux{1, 1}}, false},
+		{Cost{C: 1.0, aux: testAux{1, 1}}, Cost{C: 1.0, aux: testAux{0, 0}}, false},
 	}
 	for _, tc := range testCases {
 		if tc.left.Less(tc.right) != tc.expected {
@@ -60,8 +60,8 @@ func TestCostAdd(t *testing.T) {
 		{Cost{C: 1.0, Penalties: FullScanPenalty}, Cost{C: 2.0}, Cost{C: 3.0, Penalties: FullScanPenalty}},
 		{Cost{C: 1.0}, Cost{C: 2.0, Penalties: HugeCostPenalty}, Cost{C: 3.0, Penalties: HugeCostPenalty}},
 		{Cost{C: 1.0, Penalties: UnboundedCardinalityPenalty}, Cost{C: 2.0, Penalties: HugeCostPenalty}, Cost{C: 3.0, Penalties: HugeCostPenalty | UnboundedCardinalityPenalty}},
-		{Cost{C: 1.0, aux: testAux{1, false}}, Cost{C: 1.0, aux: testAux{2, true}}, Cost{C: 2.0, aux: testAux{3, true}}},
-		{Cost{C: 1.0, aux: testAux{200, true}}, Cost{C: 1.0, aux: testAux{100, false}}, Cost{C: 2.0, aux: testAux{255, true}}},
+		{Cost{C: 1.0, aux: testAux{1, 4}}, Cost{C: 1.0, aux: testAux{2, 5}}, Cost{C: 2.0, aux: testAux{3, 9}}},
+		{Cost{C: 1.0, aux: testAux{65530, 65530}}, Cost{C: 1.0, aux: testAux{100, 100}}, Cost{C: 2.0, aux: testAux{65535, 65535}}},
 	}
 	for _, tc := range testCases {
 		tc.left.Add(tc.right)
@@ -76,18 +76,18 @@ func TestCostSummary(t *testing.T) {
 		c        Cost
 		expected string
 	}{
-		{Cost{C: 1.0}, "1::0f"},
-		{Cost{C: 1.23}, "1.23::0f"},
-		{Cost{C: 1.23456}, "1.23456::0f"},
-		{Cost{C: 1.23, Penalties: HugeCostPenalty}, "1.23:H:0f"},
-		{Cost{C: 1.23, Penalties: FullScanPenalty}, "1.23:F:0f"},
-		{Cost{C: 1.23, Penalties: UnboundedCardinalityPenalty}, "1.23:U:0f"},
-		{Cost{C: 1.23, Penalties: HugeCostPenalty | FullScanPenalty | UnboundedCardinalityPenalty}, "1.23:HFU:0f"},
-		{Cost{C: 1.23, Penalties: HugeCostPenalty | FullScanPenalty | UnboundedCardinalityPenalty}, "1.23:HFU:0f"},
-		{Cost{C: 1.23, aux: testAux{5, false}}, "1.23::5f"},
-		{Cost{C: 1.23, aux: testAux{0, true}}, "1.23::0fu"},
-		{Cost{C: 1.23, aux: testAux{5, true}}, "1.23::5fu"},
-		{Cost{C: 1.23, Penalties: HugeCostPenalty | FullScanPenalty, aux: testAux{5, true}}, "1.23:HF:5fu"},
+		{Cost{C: 1.0}, "1::0f0u"},
+		{Cost{C: 1.23}, "1.23::0f0u"},
+		{Cost{C: 1.23456}, "1.23456::0f0u"},
+		{Cost{C: 1.23, Penalties: HugeCostPenalty}, "1.23:H:0f0u"},
+		{Cost{C: 1.23, Penalties: FullScanPenalty}, "1.23:F:0f0u"},
+		{Cost{C: 1.23, Penalties: UnboundedCardinalityPenalty}, "1.23:U:0f0u"},
+		{Cost{C: 1.23, Penalties: HugeCostPenalty | FullScanPenalty | UnboundedCardinalityPenalty}, "1.23:HFU:0f0u"},
+		{Cost{C: 1.23, Penalties: HugeCostPenalty | FullScanPenalty | UnboundedCardinalityPenalty}, "1.23:HFU:0f0u"},
+		{Cost{C: 1.23, aux: testAux{5, 0}}, "1.23::5f0u"},
+		{Cost{C: 1.23, aux: testAux{0, 6}}, "1.23::0f6u"},
+		{Cost{C: 1.23, aux: testAux{5, 10}}, "1.23::5f10u"},
+		{Cost{C: 1.23, Penalties: HugeCostPenalty | FullScanPenalty, aux: testAux{5, 9}}, "1.23:HF:5f9u"},
 	}
 	for _, tc := range testCases {
 		if r := tc.c.Summary(); r != tc.expected {