Skip to content

Commit

Permalink
randgen: do not create computed columns with bad volatilities
Browse files Browse the repository at this point in the history 
Some cast to STRING types has been given an incorrect volatility. For
example, REGCLASS->STRING casts are immutable when they should be stable
(see #74286 and #74553 for more details).

Creating computed column expressions with such a cast can cause logical
correctness bugs and internal errors. The volatilities cannot be fixed
without causing backward incompatibility. This commit prevents `randgen`
from creating computed columns with these casts so that sqlsmith and TLP
do not repetitively find these known volatility bugs.

Release note: None
  • Loading branch information
@mgartner
mgartner committed Jan 20, 2022
1 parent bed5b79 commit 8b5f53c
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 189 deletions.
25 changes: 24 additions & 1 deletion pkg/sql/randgen/expr.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,8 @@ func randExpr(

default:
volatility, ok := tree.LookupCastVolatility(xTyp, types.String, nil /* sessionData */)
if ok && volatility <= tree.VolatilityImmutable {
if ok && volatility <= tree.VolatilityImmutable &&
!typeToStringCastHasIncorrectVolatility(xTyp) {
// We can cast to string; use lower(x::string)
typ = types.String
expr = &tree.FuncExpr{
Expand Down Expand Up @@ -243,3 +244,25 @@ func randExpr(

return expr, typ, nullability
}

// typeToStringCastHasIncorrectVolatility returns true for a given type if the
// cast from it to STRING types has been given an incorrect volatility. For
// example, REGCLASS->STRING casts are immutable when they should be stable (see
// #74286 and #74553 for more details).
//
// Creating computed column expressions with such a cast can cause logical
// correctness bugs and internal errors. The volatilities cannot be fixed
// without causing backward incompatibility, so this function is used to prevent
// sqlsmith and TLP from repetitively finding these known volatility bugs.
func typeToStringCastHasIncorrectVolatility(t *types.T) bool {
switch t.Family() {
case types.DateFamily, types.EnumFamily, types.TimestampFamily,
types.IntervalFamily, types.TupleFamily:
return true
case types.OidFamily:
return t == types.RegClass || t == types.RegNamespace || t == types.RegProc ||
t == types.RegProcedure || t == types.RegRole || t == types.RegType
default:
return false
}
}
43 changes: 0 additions & 43 deletions pkg/sql/randgen/mutator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -768,8 +768,6 @@ func postgresCreateTableMutator(
if colTypeFamily == types.Box2DFamily {
isBox2d = true
}
} else {
col.Expr, changed = replaceNonImmutableExpr(rng, col.Expr, colTypes)
}
if isBox2d {
changed = true
Expand Down Expand Up @@ -808,11 +806,6 @@ func postgresCreateTableMutator(
Storing: def.Storing,
})
changed = true
case *tree.ColumnTableDef:
if def.IsComputed() {
def.Computed.Expr, changed = replaceNonImmutableExpr(rng, def.Computed.Expr, colTypes)
}
newdefs = append(newdefs, def)
default:
newdefs = append(newdefs, def)
}
Expand All @@ -823,42 +816,6 @@ func postgresCreateTableMutator(
return mutated, changed
}

// replaceNonImmutableExpr checks if expr has a non-immutable operation in it,
// and returns an immutable expr if it does. This is needed because Postgres has
// different cast volatility for timestamps and/OID types. The substitution here
// is specific to the output of randgen.randExpr, which uses
// `lower(cast(col as string))`.
func replaceNonImmutableExpr(
rng *rand.Rand, expr tree.Expr, colTypes map[string]*types.T,
) (newExpr tree.Expr, changed bool) {
if funcExpr, ok := expr.(*tree.FuncExpr); ok {
if len(funcExpr.Exprs) == 1 {
if castExpr, ok := funcExpr.Exprs[0].(*tree.CastExpr); ok {
referencedType := colTypes[castExpr.Expr.(*tree.UnresolvedName).String()]
isContextDependentType := referencedType.Family() == types.TimestampFamily ||
referencedType.Family() == types.OidFamily ||
referencedType.Family() == types.DateFamily
if isContextDependentType &&
tree.MustBeStaticallyKnownType(castExpr.Type) == types.String {
newExpr = &tree.CaseExpr{
Whens: []*tree.When{
{
Cond: &tree.IsNullExpr{
Expr: castExpr.Expr,
},
Val: RandDatum(rng, types.String, true /* nullOK */),
},
},
Else: RandDatum(rng, types.String, true /* nullOK */),
}
return newExpr, true
}
}
}
}
return expr, false
}

// columnFamilyMutator is mutations.StatementMutator, but lives here to prevent
// dependency cycles with RandCreateTable.
func columnFamilyMutator(rng *rand.Rand, stmt tree.Statement) (changed bool) {
Expand Down
145 changes: 0 additions & 145 deletions pkg/sql/randgen/mutator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,151 +17,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/randutil"
)

func TestPostgresCreateTableMutator(t *testing.T) {
q := `
CREATE TABLE table1 (
col1_0
TIMESTAMP,
col1_1
REGPROC,
col1_2
BOX2D NOT NULL,
col1_3
"char" NOT NULL,
col1_4
GEOGRAPHY NULL,
col1_5
GEOGRAPHY,
col1_6
REGROLE NOT NULL,
col1_7
REGROLE NOT NULL,
col1_8
"char",
col1_9
INTERVAL,
col1_10
STRING AS (lower(col1_3)) STORED NOT NULL,
col1_11
STRING AS (lower(CAST(col1_1 AS STRING))) STORED,
col1_12
STRING AS (lower(CAST(col1_5 AS STRING))) STORED,
col1_13
STRING AS (lower(CAST(col1_0 AS STRING))) VIRTUAL,
col1_14
STRING AS (lower(CAST(col1_4 AS STRING))) STORED NULL,
col1_15
STRING AS (lower(CAST(col1_7 AS STRING))) STORED NOT NULL,
col1_16
STRING AS (lower(CAST(col1_0 AS STRING))) STORED,
INDEX (col1_9, col1_1, col1_12 ASC)
WHERE
(
(table1.col1_8 != 'X':::STRING OR table1.col1_16 < e'\x00':::STRING)
AND table1.col1_15 <= '':::STRING
)
AND table1.col1_14 >= e'\U00002603':::STRING,
UNIQUE (
col1_1 ASC,
lower(CAST(col1_5 AS STRING)) ASC,
col1_0,
col1_10,
col1_11 ASC,
col1_12 ASC,
col1_2 ASC,
col1_7,
col1_3,
col1_6,
col1_16
)
WHERE
(
(
(
(
(
(
(
(
table1.col1_0 < '-4713-11-24 00:00:00':::TIMESTAMP
AND table1.col1_14 = e'\'':::STRING
)
AND table1.col1_3 = '"':::STRING
)
OR table1.col1_8 >= e'\U00002603':::STRING
)
OR table1.col1_13 > '':::STRING
)
AND table1.col1_12 < '':::STRING
)
OR table1.col1_11 >= 'X':::STRING
)
AND table1.col1_10 <= e'\'':::STRING
)
OR table1.col1_16 != '':::STRING
)
OR table1.col1_15 >= '':::STRING,
UNIQUE (col1_1, col1_16 DESC, col1_6, col1_11 DESC, col1_9 ASC, col1_3 DESC, col1_2 ASC),
UNIQUE (col1_14 DESC, col1_15 DESC)
WHERE
(
(
(
(
(
(
(
(table1.col1_14 >= e'\'':::STRING OR table1.col1_16 >= 'X':::STRING)
OR table1.col1_3 <= 'X':::STRING
)
AND table1.col1_15 = 'X':::STRING
)
OR table1.col1_13 > '"':::STRING
)
OR table1.col1_12 >= '':::STRING
)
AND table1.col1_11 != '':::STRING
)
OR table1.col1_10 != e'\U00002603':::STRING
)
AND table1.col1_8 != e'\U00002603':::STRING
)
OR table1.col1_0 <= '-4713-11-24 00:00:00':::TIMESTAMP,
INVERTED INDEX (
col1_13,
col1_16,
col1_12,
col1_8,
col1_3 ASC,
col1_6,
col1_2,
col1_7,
col1_0 DESC,
lower(CAST(col1_7 AS STRING)) DESC,
col1_1,
col1_15 DESC,
col1_4
)
)`
rng, _ := randutil.NewTestRand()
// Set a deterministic seed so that PostgresCreateTableMutator performs a
// deterministic transformation.
rng.Seed(123)
mutated, changed := ApplyString(rng, q, PostgresCreateTableMutator)
if !changed {
t.Fatal("expected changed")
}
expect := `
CREATE TABLE table1 (col1_0 TIMESTAMP, col1_1 REGPROC, col1_2 BOX2D NOT NULL, col1_3 "char" NOT NULL, col1_4 GEOGRAPHY NULL, col1_5 GEOGRAPHY, col1_6 REGROLE NOT NULL, col1_7 REGROLE NOT NULL, col1_8 "char", col1_9 INTERVAL, col1_10 STRING NOT NULL AS (lower(col1_3)) STORED, col1_11 STRING AS (CASE WHEN col1_1 IS NULL THEN 'L*h':::STRING ELSE '#':::STRING END) STORED, col1_12 STRING AS (lower(CAST(col1_5 AS STRING))) STORED, col1_13 STRING AS (CASE WHEN col1_0 IS NULL THEN e'\x1c\t':::STRING ELSE e'(,Zh\x05\x1dW':::STRING END) VIRTUAL, col1_14 STRING NULL AS (lower(CAST(col1_4 AS STRING))) STORED, col1_15 STRING NOT NULL AS (CASE WHEN col1_7 IS NULL THEN e'\x0e,\x162/BJ\x12':::STRING ELSE e'#\x17\x07;\x0emi':::STRING END) STORED, col1_16 STRING AS (CASE WHEN col1_0 IS NULL THEN e'\x1eM\x02\x12_\x05\r':::STRING ELSE e'[jUDO\nt':::STRING END) STORED);
CREATE INDEX ON table1 (col1_9, col1_1, col1_12 ASC);
CREATE UNIQUE INDEX ON table1 (col1_1 ASC, lower(CAST(col1_5 AS STRING)) ASC, col1_0, col1_10, col1_11 ASC, col1_12 ASC, col1_7, col1_3, col1_6, col1_16);
CREATE UNIQUE INDEX ON table1 (col1_1, col1_16 DESC, col1_6, col1_11 DESC, col1_9 ASC, col1_3 DESC);
CREATE UNIQUE INDEX ON table1 (col1_14 DESC, col1_15 DESC);`
if strings.TrimSpace(mutated) != strings.TrimSpace(expect) {
t.Fatalf("expected:\n%s\ngot:\n%s", expect, mutated)
}
}

func TestPostgresMutator(t *testing.T) {
q := `
CREATE TABLE t (s STRING FAMILY fam1, b BYTES, FAMILY fam2 (b), PRIMARY KEY (s ASC, b DESC), INDEX (s) STORING (b))
Expand Down

0 comments on commit 8b5f53c

Please sign in to comment.