Skip to content

Commit

Permalink
Merge #119652 #120984 #121294
Browse files Browse the repository at this point in the history 
119652: roachtest: assert limit capacity throughput 90% of baseline r=andrewbaptist a=kvoli

This commit enables a throughput assertion on the `limit_capacity`
roachtests. The assertion requires that throughput after limiting some
nodes capacity is at least 90% of the pre-limit throughput after 2
minutes.

Part of: #118866
Release note: None

120984: sql: alter sequence options no min, no max and as type to replicate postgreSQL r=rafiss a=andrew-delph

This commit changes 'NO MINVALUE' and 'NO MAXVALUE' sequence options
to replicate postgreSQL behavior.
If the sequence is ascending then the NO MAXVALUE is the largest value
for it int type and NO MINVALUE is 1.
If the sequence is descending then the NO MAXVALUE is -1 and NO MINVALUE
is the smallest value for its int type.
This commit also addresses how MINVALUE and MAXVALUE are automatically
changed when altering the seq int type. If either is equal to the
current types limit, then it is will automatically adjsut to the
changes types limits. Otherwise it will not change.

Epic: None

Release note (bug fix): Sequence options for 'NO MINVALUE' and 'NO
MAXVALUE' now replicate postgreSQL.

Sequence MINVALUE and MAXVALUE automatical adjust to new types bounds
mirroring behavior of postgreSQL.

121294: pkg/sql/sqlstats: de-flake TestTransactionInsightsFailOnCommit r=dhartunian a=abarganier

Fixes: #117061

Epic: none

TestTransactionInsightsFailOnCommit runs two transactions, one of which is expected to fail on commit due to an isolation error as it interacts with the other transaction.

We query crdb_internal.node_txn_execution_insights, which queries the in-memory insights data, to check that a txn insight was generated for the failed txn with a `FailedExecution` insight.

However, when querying the crdb_internal table, the query is too broad. In the test, the successful transaction can still possibly generate a `SlowExecution` insight. In this case, when looking for the `FailedExecution` insight, we were instead getting an insight from the successful transaction.

The fix is to narrow the SELECT criteria to ensure we're selecting the correct insight.

The fix also expands the failed assertion logging.

Release note: none

Co-authored-by: Austen McClernon <[email protected]>
Co-authored-by: Andrew Delph <[email protected]>
Co-authored-by: Alex Barganier <[email protected]>
  • Loading branch information
@kvoli @andrew-delph @abarganier
4 people committed Mar 28, 2024
4 parents 22df9ee + d8f2742 + 3a8bc70 + eb77666 commit 5470a6b
Show file tree
Hide file tree
Showing 4 changed files with 145 additions and 93 deletions.
13 changes: 2 additions & 11 deletions pkg/cmd/roachtest/tests/limit_capacity.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,6 @@ func runLimitCapacity(ctx context.Context, t test.Test, c cluster.Cluster, cfg l
for _, cancel := range cancels {
cancel()
}
// We should be able to assert on the throughput not dropping beyond a
// certain % of the throughput prior to limiting a node's capacity.
//
// TODO(kvoli): Currently this test will fail an assertion that the final QPS
// will be >50% of the pre-limit QPS. Once we begin shedding leases off the
// limited node, this assertion would pass. Add in these assertions once
// shedding is done, or alternatively enable the test weekly and export the
// relative QPS to roachperf. Two potential assertions are:
//
// (a) expect throughput to not drop by more than X%
// (b) measure the throughput at set marks (10s, 30s, 1m, 5m) and assert.
// Expect that the relative QPS is at least 90% of the starting QPS.
require.GreaterOrEqual(t, qpsRelative, 0.9)
}
106 changes: 27 additions & 79 deletions pkg/sql/catalog/schemaexpr/sequence_options.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,57 +72,6 @@ func getSequenceIntegerBounds(
)
}

func setSequenceIntegerBounds(
opts *descpb.TableDescriptor_SequenceOpts,
integerType *types.T,
isAscending bool,
setMinValue bool,
setMaxValue bool,
) error {
var minValue int64 = math.MinInt64
var maxValue int64 = math.MaxInt64

if isAscending {
minValue = 1

switch integerType {
case types.Int2:
maxValue = math.MaxInt16
case types.Int4:
maxValue = math.MaxInt32
case types.Int:
// Do nothing, it's the default.
default:
return errors.AssertionFailedf(
"CREATE SEQUENCE option AS received type %s, must be integer",
integerType,
)
}
} else {
maxValue = -1
switch integerType {
case types.Int2:
minValue = math.MinInt16
case types.Int4:
minValue = math.MinInt32
case types.Int:
// Do nothing, it's the default.
default:
return errors.AssertionFailedf(
"CREATE SEQUENCE option AS received type %s, must be integer",
integerType,
)
}
}
if setMinValue {
opts.MinValue = minValue
}
if setMaxValue {
opts.MaxValue = maxValue
}
return nil
}

// AssignSequenceOptions moves options from the AST node to the sequence options descriptor,
// starting with defaults and overriding them with user-provided options.
func AssignSequenceOptions(
Expand All @@ -132,7 +81,6 @@ func AssignSequenceOptions(
setDefaults bool,
existingType *types.T,
) error {
wasAscending := opts.Increment > 0

// Set the default integer type of a sequence.
integerType := parser.NakedIntTypeFromDefaultIntSize(defaultIntSize)
Expand Down Expand Up @@ -171,32 +119,20 @@ func AssignSequenceOptions(
opts.CacheSize = 1
}

// Set default MINVALUE and MAXVALUE if AS option value for integer type is specified.
if opts.AsIntegerType != "" {
// We change MINVALUE and MAXVALUE if it is the originally set to the default during ALTER.
setMinValue := setDefaults
setMaxValue := setDefaults
if !setDefaults && existingType != nil {
existingLowerIntBound, existingUpperIntBound, err := getSequenceIntegerBounds(existingType)
if err != nil {
return err
}
if (wasAscending && opts.MinValue == 1) || (!wasAscending && opts.MinValue == existingLowerIntBound) {
setMinValue = true
}
if (wasAscending && opts.MaxValue == existingUpperIntBound) || (!wasAscending && opts.MaxValue == -1) {
setMaxValue = true
}
// Set Minvalue and Maxvalue to new types bounds if at current bounds.
if !setDefaults && existingType != nil && opts.AsIntegerType != "" {
existingLowerIntBound, existingUpperIntBound, err := getSequenceIntegerBounds(existingType)
if err != nil {
return err
}
// If Minvalue is bounded to the existing type, set it to the bounds of the new type.
if opts.MinValue == existingLowerIntBound {
opts.MinValue = lowerIntBound
}

if err := setSequenceIntegerBounds(
opts,
integerType,
isAscending,
setMinValue,
setMaxValue,
); err != nil {
return err
// If MaxValue is bounded to the existing type, set it to the bounds of the new type.
if opts.MaxValue == existingUpperIntBound {
opts.MaxValue = upperIntBound
}
}

Expand Down Expand Up @@ -235,12 +171,24 @@ func AssignSequenceOptions(
// Do nothing; this has already been set.
case tree.SeqOptMinValue:
// A value of nil represents the user explicitly saying `NO MINVALUE`.
if option.IntVal != nil {
if option.IntVal == nil {
if isAscending {
opts.MinValue = 1
} else {
opts.MinValue = lowerIntBound
}
} else {
opts.MinValue = *option.IntVal
}
case tree.SeqOptMaxValue:
// A value of nil represents the user explicitly saying `NO MAXVALUE`.
if option.IntVal != nil {
if option.IntVal == nil {
if isAscending {
opts.MaxValue = upperIntBound
} else {
opts.MaxValue = -1
}
} else {
opts.MaxValue = *option.IntVal
}
case tree.SeqOptStart:
Expand All @@ -253,7 +201,7 @@ func AssignSequenceOptions(
}
}

if setDefaults || (wasAscending && opts.Start == 1) || (!wasAscending && opts.Start == -1) {
if setDefaults {
// If start option not specified, set it to MinValue (for ascending sequences)
// or MaxValue (for descending sequences).
// We only do this if we're setting it for the first time, or the sequence was
Expand Down
111 changes: 110 additions & 1 deletion pkg/sql/logictest/testdata/logic_test/alter_sequence
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ ALTER SEQUENCE reverse_direction_seqas AS smallint INCREMENT -1
query T
SELECT create_statement FROM [SHOW CREATE SEQUENCE reverse_direction_seqas]
----
CREATE SEQUENCE public.reverse_direction_seqas AS INT2 MINVALUE -32768 MAXVALUE -1 INCREMENT -1 START -1
CREATE SEQUENCE public.reverse_direction_seqas AS INT2 MINVALUE 1 MAXVALUE 32767 INCREMENT -1 START 1

statement ok
ALTER SEQUENCE reverse_direction_seqas AS int INCREMENT 1
Expand Down Expand Up @@ -286,3 +286,112 @@ CREATE SEQUENCE restart_max_err_seqas MAXVALUE 100

statement error RESTART value \(1000\) cannot be greater than MAXVALUE \(100\)
ALTER SEQUENCE restart_max_err_seqas RESTART 1000


statement ok
CREATE SEQUENCE set_no_maxvalue MAXVALUE 3

query III
select nextval('set_no_maxvalue'),nextval('set_no_maxvalue'),nextval('set_no_maxvalue')
----
1 2 3

statement error pgcode 2200H pq: nextval\(\): reached maximum value of sequence "set_no_maxvalue" \(3\)
select nextval('set_no_maxvalue')

statement ok
ALTER SEQUENCE set_no_maxvalue NO MAXVALUE

query I
select nextval('set_no_maxvalue')
----
4

statement ok
CREATE SEQUENCE set_no_minvalue INCREMENT -1 MINVALUE -3;

query III
select nextval('set_no_minvalue'),nextval('set_no_minvalue'),nextval('set_no_minvalue')
----
-1 -2 -3

statement error pgcode 2200H pq: nextval\(\): reached minimum value of sequence "set_no_minvalue" \(-3\)
select nextval('set_no_minvalue')

statement ok
ALTER SEQUENCE set_no_minvalue NO MINVALUE

query I
select nextval('set_no_minvalue')
----
-4


statement ok
CREATE SEQUENCE seq_inverse_no_change;

statement ok
ALTER SEQUENCE seq_inverse_no_change INCREMENT BY -1;

query TT colnames
SHOW CREATE SEQUENCE seq_inverse_no_change;
----
table_name create_statement
seq_inverse_no_change CREATE SEQUENCE public.seq_inverse_no_change MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT -1 START 1

statement ok
ALTER SEQUENCE seq_inverse_no_change INCREMENT BY 1;

query TT colnames
SHOW CREATE SEQUENCE seq_inverse_no_change;
----
table_name create_statement
seq_inverse_no_change CREATE SEQUENCE public.seq_inverse_no_change MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 1


statement ok
CREATE SEQUENCE seq_change_type_bounds MINVALUE -9223372036854775808 MAXVALUE 9223372036854775807 START 0

statement ok
ALTER SEQUENCE seq_change_type_bounds AS smallint;

query TT colnames
SHOW CREATE SEQUENCE seq_change_type_bounds;
----
table_name create_statement
seq_change_type_bounds CREATE SEQUENCE public.seq_change_type_bounds AS INT2 MINVALUE -32768 MAXVALUE 32767 INCREMENT 1 START 0

statement ok
ALTER SEQUENCE seq_change_type_bounds AS integer;

query TT colnames
SHOW CREATE SEQUENCE seq_change_type_bounds;
----
table_name create_statement
seq_change_type_bounds CREATE SEQUENCE public.seq_change_type_bounds AS INT8 MINVALUE -9223372036854775808 MAXVALUE 9223372036854775807 INCREMENT 1 START 0


statement ok
CREATE SEQUENCE seq_alter_no_min_max_asc INCREMENT 1 START 5 MINVALUE -10 MAXVALUE 10;

statement ok
ALTER SEQUENCE seq_alter_no_min_max_asc NO MINVALUE NO MAXVALUE;

query TT colnames
SHOW CREATE SEQUENCE seq_alter_no_min_max_asc;
----
table_name create_statement
seq_alter_no_min_max_asc CREATE SEQUENCE public.seq_alter_no_min_max_asc MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 5


statement ok
CREATE SEQUENCE seq_alter_no_min_max_des INCREMENT -1 START -5 MINVALUE -10 MAXVALUE 10 ;

statement ok
ALTER SEQUENCE seq_alter_no_min_max_des NO MINVALUE NO MAXVALUE ;

query TT colnames
SHOW CREATE SEQUENCE seq_alter_no_min_max_des
----
table_name create_statement
seq_alter_no_min_max_des CREATE SEQUENCE public.seq_alter_no_min_max_des MINVALUE -9223372036854775808 MAXVALUE -1 INCREMENT -1 START -5
8 changes: 6 additions & 2 deletions pkg/sql/sqlstats/insights/integration/insights_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -490,12 +490,16 @@ SELECT query,
COALESCE(last_error_code, '') last_error_code,
COALESCE(last_error_redactable, '') last_error
FROM crdb_internal.node_txn_execution_insights
WHERE app_name = $1`, appName)
WHERE app_name = $1
AND query = 'SELECT * FROM myusers WHERE city = _ ; UPDATE myusers SET name = _ WHERE city = _'`, appName)

return row.Scan(&query, &problems, &status, &errorCode, &errorMsg)
})

require.Equal(t, "SELECT * FROM myusers WHERE city = _ ; UPDATE myusers SET name = _ WHERE city = _", query)
require.Equalf(t,
"SELECT * FROM myusers WHERE city = _ ; UPDATE myusers SET name = _ WHERE city = _", query,
"unexpected txn insight found - query: %s, problems: %s, status: %s, errCode: %s, errMsg: %s",
query, problems, status, errorCode, errorMsg)
expectedProblem := "{FailedExecution}"
replacedSlowProblems := problems
if problems != expectedProblem {
Expand Down

0 comments on commit 5470a6b

Please sign in to comment.