Skip to content

Commit

Permalink
workload/schemachanger: unique constraint validation is buggy for exprs
Browse files Browse the repository at this point in the history
Previously, when we were validating unique constraint expressions during
inserts we did not properly handle NULL values. This could lead to us
incorrectly skipping over evaluation trying to determine the uniqueness
of values. This could mean both valid / invalid inserts were not properly
fully evaluated leading to unexpected errors. To address this, this
patch properly counts the number of NULL values in the unique expression
where if all of them are NULL then the expression is not evaluated for
uniqueness.

Release note: None
  • Loading branch information
fqazi committed Jun 14, 2022
1 parent 63aabd9 commit c0af62a
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 19 deletions.
1 change: 0 additions & 1 deletion pkg/cmd/roachtest/tests/schemachange_random_load.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ func registerSchemaChangeRandomLoad(r registry.Registry) {
}
runSchemaChangeRandomLoad(ctx, t, c, maxOps, concurrency)
},
Skip: "flaky: https://github.com/cockroachdb/cockroach/issues/82133",
})

// Run a few representative scbench specs in CI.
Expand Down
69 changes: 51 additions & 18 deletions pkg/workload/schemachange/error_screening.go
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,17 @@ GROUP BY name;
}
// Next validate the uniqueness of both constraints and index expressions.
for constraintIdx, constraint := range constraints {
nonTupleConstraint := constraint[0]
if len(nonTupleConstraint) > 2 &&
nonTupleConstraint[0] == '(' &&
nonTupleConstraint[len(nonTupleConstraint)-1] == ')' {
nonTupleConstraint = nonTupleConstraint[1 : len(nonTupleConstraint)-1]
}
hasNullsQuery := strings.Builder{}
hasNullsQuery.WriteString("SELECT num_nulls(")
hasNullsQuery.WriteString(nonTupleConstraint)
hasNullsQuery.WriteString(") > 0 FROM (VALUES(")

tupleSelectQuery := strings.Builder{}
tupleSelectQuery.WriteString("SELECT array[(")
tupleSelectQuery.WriteString(constraint[0])
Expand All @@ -361,7 +372,7 @@ GROUP BY name;
}
collector := newExprColumnCollector(colInfo)
t.Walk(collector)
query.WriteString("SELECT EXISTS ( SELECT * FROM ")
query.WriteString("SELECT COUNT (*) > 0 FROM (SELECT * FROM ")
query.WriteString(tableName.String())
query.WriteString(" WHERE ")
query.WriteString(constraint[0])
Expand All @@ -370,30 +381,23 @@ GROUP BY name;
query.WriteString(constraint[0])
query.WriteString(" FROM (VALUES( ")
colIdx := 0
nullValueEncountered := false
for col := range collector.columnsObserved {
value := columnsToValues[col]
if colIdx != 0 {
query.WriteString(",")
columns.WriteString(",")
tupleSelectQuery.WriteString(",")
}
if value == "NULL" {
nullValueEncountered = true
break
hasNullsQuery.WriteString(",")
}
query.WriteString(value)
columns.WriteString(col)
hasNullsQuery.WriteString(value)
tupleSelectQuery.WriteString(value)
colIdx++
}
// Row is not comparable to others for unique constraints, since it has a
// NULL value.
// TODO (fqazi): In the future for check constraints we should evaluate
// things for them.
if nullValueEncountered {
continue
}
hasNullsQuery.WriteString(") ) AS T(")
hasNullsQuery.WriteString(columns.String())
hasNullsQuery.WriteString(")")
tupleSelectQuery.WriteString(") ) AS T(")
tupleSelectQuery.WriteString(columns.String())
tupleSelectQuery.WriteString(")")
Expand All @@ -404,24 +408,53 @@ GROUP BY name;
if err != nil {
return false, nil, err
}
exists, err := og.scanBool(ctx, evalTxn, query.String())
if err != nil {
// Detect if any null values exist.
handleEvalTxnError := func(err error) (bool, error) {
// No choice but to rollback, expression is malformed.
rollbackErr := evalTxn.Rollback(ctx)
if rollbackErr != nil {
return false, err
}
var pgErr *pgconn.PgError
if !errors.As(err, &pgErr) {
return false, nil, err
return false, err
}
// Only accept known error types for generated expressions.
if !isValidGenerationError(pgErr.Code) {
return false, nil, err
return false, err
}
generatedCodes = append(generatedCodes,
codesWithConditions{
{code: pgcode.MakeCode(pgErr.Code), condition: true},
}...,
)
return true, nil
}
hasNullValues, err := og.scanBool(ctx, evalTxn, hasNullsQuery.String())
if err != nil {
skipConstraint, err := handleEvalTxnError(err)
if err != nil {
return false, generatedCodes, err
}
if skipConstraint {
continue
}
}
// Skip if any null values exist in the expression
if hasNullValues {
continue
}
err = evalTxn.Rollback(ctx)
exists, err := og.scanBool(ctx, evalTxn, query.String())
if err != nil {
skipConstraint, err := handleEvalTxnError(err)
if err != nil {
return false, generatedCodes, err
}
if skipConstraint {
continue
}
}
err = evalTxn.Commit(ctx)
if err != nil {
return false, nil, err
}
Expand Down

0 comments on commit c0af62a

Please sign in to comment.