From 57a01a57711ec826355d9a4063293d00b5dc20a1 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Wed, 17 Nov 2021 20:04:46 +1100 Subject: [PATCH] sql: fix ALTER SEQUENCE ... AS ... defaults Previously, ALTER SEQUENCE ... AS ... can change MINVALUE and MAXVALUE to the new type value. This is now preserved and errors if values are out of range. Release note: None --- pkg/sql/alter_sequence.go | 28 ++- pkg/sql/create_sequence.go | 11 +- .../testdata/logic_test/alter_sequence | 171 +++++++++++++++++ pkg/sql/sequence.go | 176 +++++++++++++----- 4 files changed, 335 insertions(+), 51 deletions(-) diff --git a/pkg/sql/alter_sequence.go b/pkg/sql/alter_sequence.go index 59037f32a157..3f30d2bc58f1 100644 --- a/pkg/sql/alter_sequence.go +++ b/pkg/sql/alter_sequence.go @@ -19,7 +19,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" + "github.com/cockroachdb/errors" ) type alterSequenceNode struct { @@ -66,10 +68,28 @@ func (n *alterSequenceNode) startExec(params runParams) error { oldMinValue := desc.SequenceOpts.MinValue oldMaxValue := desc.SequenceOpts.MaxValue - err := assignSequenceOptions( - desc.SequenceOpts, n.n.Options, false /* setDefaults */, ¶ms, desc.GetID(), desc.ParentID, - ) - if err != nil { + existingType := types.Int + if desc.GetSequenceOpts().AsIntegerType != "" { + switch desc.GetSequenceOpts().AsIntegerType { + case types.Int2.SQLString(): + existingType = types.Int2 + case types.Int4.SQLString(): + existingType = types.Int4 + case types.Int.SQLString(): + // Already set. + default: + return errors.AssertionFailedf("sequence has unexpected type %s", desc.GetSequenceOpts().AsIntegerType) + } + } + if err := assignSequenceOptions( + desc.SequenceOpts, + n.n.Options, + false, /* setDefaults */ + ¶ms, + desc.GetID(), + desc.ParentID, + existingType, + ); err != nil { return err } opts := desc.SequenceOpts diff --git a/pkg/sql/create_sequence.go b/pkg/sql/create_sequence.go index 6f77398beea1..ed5ee3a106df 100644 --- a/pkg/sql/create_sequence.go +++ b/pkg/sql/create_sequence.go @@ -222,8 +222,15 @@ func NewSequenceTableDesc( opts := &descpb.TableDescriptor_SequenceOpts{ Increment: 1, } - err := assignSequenceOptions(opts, sequenceOptions, true /* setDefaults */, params, id, parentID) - if err != nil { + if err := assignSequenceOptions( + opts, + sequenceOptions, + true, /* setDefaults */ + params, + id, + parentID, + nil, /* existingType */ + ); err != nil { return nil, err } desc.SequenceOpts = opts diff --git a/pkg/sql/logictest/testdata/logic_test/alter_sequence b/pkg/sql/logictest/testdata/logic_test/alter_sequence index 237a2c6695af..e4ed371c0d62 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_sequence +++ b/pkg/sql/logictest/testdata/logic_test/alter_sequence @@ -60,3 +60,174 @@ query T SELECT create_statement FROM [SHOW CREATE SEQUENCE foo] ---- CREATE SEQUENCE public.foo MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 5 START 1 + +user root + +# Alter between sequences, check it is ok. +statement ok +CREATE SEQUENCE seq_as AS int2 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_as] +---- +CREATE SEQUENCE public.seq_as AS INT2 MINVALUE 1 MAXVALUE 32767 INCREMENT 1 START 1 + +statement ok +ALTER SEQUENCE seq_as AS int4 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_as] +---- +CREATE SEQUENCE public.seq_as AS INT4 MINVALUE 1 MAXVALUE 2147483647 INCREMENT 1 START 1 + +statement ok +ALTER SEQUENCE seq_as AS int8 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_as] +---- +CREATE SEQUENCE public.seq_as AS INT8 MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 1 + +statement ok +ALTER SEQUENCE seq_as AS int4 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_as] +---- +CREATE SEQUENCE public.seq_as AS INT4 MINVALUE 1 MAXVALUE 2147483647 INCREMENT 1 START 1 + +statement ok +ALTER SEQUENCE seq_as AS int2 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_as] +---- +CREATE SEQUENCE public.seq_as AS INT2 MINVALUE 1 MAXVALUE 32767 INCREMENT 1 START 1 + +# Test ALTER SEQUENCE AS when downgrading sizes behaves appropriately if MINVALUE/MAXVALUE/START is set. + +statement ok +CREATE SEQUENCE seq_int4_max_high AS int4 MAXVALUE 99999 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_int4_max_high] +---- +CREATE SEQUENCE public.seq_int4_max_high AS INT4 MINVALUE 1 MAXVALUE 99999 INCREMENT 1 START 1 + +statement error MAXVALUE \(99999\) must be less than \(32767\) for type INT2 +ALTER SEQUENCE seq_int4_max_high AS int2 + +statement ok +ALTER SEQUENCE seq_int4_max_high AS int8 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_int4_max_high] +---- +CREATE SEQUENCE public.seq_int4_max_high AS INT8 MINVALUE 1 MAXVALUE 99999 INCREMENT 1 START 1 + +statement ok +CREATE SEQUENCE seq_int4_min_high AS int4 MINVALUE 99999 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_int4_min_high] +---- +CREATE SEQUENCE public.seq_int4_min_high AS INT4 MINVALUE 99999 MAXVALUE 2147483647 INCREMENT 1 START 99999 + +statement error MINVALUE \(99999\) must be less than \(32767\) for type INT2 +ALTER SEQUENCE seq_int4_min_high AS int2 + +statement ok +ALTER SEQUENCE seq_int4_min_high AS int8 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_int4_min_high] +---- +CREATE SEQUENCE public.seq_int4_min_high AS INT8 MINVALUE 99999 MAXVALUE 9223372036854775807 INCREMENT 1 START 99999 + +statement ok +CREATE SEQUENCE seq_int4_start_high AS int4 START 99999 + +statement error START value \(99999\) cannot be greater than MAXVALUE \(32767\) +ALTER SEQUENCE seq_int4_start_high AS int2 + +statement ok +ALTER SEQUENCE seq_int4_start_high AS int8 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_int4_start_high] +---- +CREATE SEQUENCE public.seq_int4_start_high AS INT8 MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 99999 + +statement ok +CREATE SEQUENCE seq_int4_min_low AS int4 MINVALUE -99999 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_int4_min_low] +---- +CREATE SEQUENCE public.seq_int4_min_low AS INT4 MINVALUE -99999 MAXVALUE 2147483647 INCREMENT 1 START -99999 + +statement error MINVALUE \(-99999\) must be greater than \(-32768\) for type INT2 +ALTER SEQUENCE seq_int4_min_low AS int2 + +statement ok +ALTER SEQUENCE seq_int4_min_low AS int8 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_int4_min_low] +---- +CREATE SEQUENCE public.seq_int4_min_low AS INT8 MINVALUE -99999 MAXVALUE 9223372036854775807 INCREMENT 1 START -99999 + +statement ok +CREATE SEQUENCE seq_int4_max_high_desc AS int4 MAXVALUE 99999 INCREMENT -1 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_int4_max_high_desc] +---- +CREATE SEQUENCE public.seq_int4_max_high_desc AS INT4 MINVALUE -2147483648 MAXVALUE 99999 INCREMENT -1 START 99999 + +statement error MAXVALUE \(99999\) must be less than \(32767\) for type INT2 +ALTER SEQUENCE seq_int4_max_high_desc AS int2 + +statement ok +ALTER SEQUENCE seq_int4_max_high_desc AS int8 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_int4_max_high_desc] +---- +CREATE SEQUENCE public.seq_int4_max_high_desc AS INT8 MINVALUE -9223372036854775808 MAXVALUE 99999 INCREMENT -1 START 99999 + +statement ok +CREATE SEQUENCE seq_int4_min_high_desc AS int4 MINVALUE -99999 INCREMENT -1 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_int4_min_high_desc] +---- +CREATE SEQUENCE public.seq_int4_min_high_desc AS INT4 MINVALUE -99999 MAXVALUE -1 INCREMENT -1 START -1 + +statement error MINVALUE \(-99999\) must be greater than \(-32768\) for type INT2 +ALTER SEQUENCE seq_int4_min_high_desc AS int2 + +statement ok +ALTER SEQUENCE seq_int4_min_high_desc AS int8 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE seq_int4_min_high_desc] +---- +CREATE SEQUENCE public.seq_int4_min_high_desc AS INT8 MINVALUE -99999 MAXVALUE -1 INCREMENT -1 START -1 + +statement ok +CREATE SEQUENCE reverse_direction_seqas AS integer; +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 + +statement ok +ALTER SEQUENCE reverse_direction_seqas AS int INCREMENT 1 + +query T +SELECT create_statement FROM [SHOW CREATE SEQUENCE reverse_direction_seqas] +---- +CREATE SEQUENCE public.reverse_direction_seqas AS INT8 MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 1 diff --git a/pkg/sql/sequence.go b/pkg/sql/sequence.go index 13aa750fdc4a..30770858a09c 100644 --- a/pkg/sql/sequence.go +++ b/pkg/sql/sequence.go @@ -363,52 +363,75 @@ func readOnlyError(s string) error { "cannot execute %s in a read-only transaction", s) } +func getSequenceIntegerBounds( + integerType *types.T, +) (lowerIntBound int64, upperIntBound int64, err error) { + switch integerType { + case types.Int2: + return math.MinInt16, math.MaxInt16, nil + case types.Int4: + return math.MinInt32, math.MaxInt32, nil + case types.Int: + return math.MinInt64, math.MaxInt64, nil + } + + return 0, 0, pgerror.Newf( + pgcode.InvalidParameterValue, + "CREATE SEQUENCE option AS received type %s, must be integer", + integerType, + ) +} + func setSequenceIntegerBounds( opts *descpb.TableDescriptor_SequenceOpts, integerType *types.T, isAscending bool, - lowerIntBound *int64, - upperIntBound *int64, + setMinValue bool, + setMaxValue bool, ) error { - opts.MinValue = math.MinInt64 - opts.MaxValue = math.MaxInt64 + var minValue int64 = math.MinInt64 + var maxValue int64 = math.MaxInt64 if isAscending { - opts.MinValue = 1 + minValue = 1 switch integerType { case types.Int2: - opts.MaxValue = math.MaxInt16 - *upperIntBound = math.MaxInt16 - *lowerIntBound = math.MinInt16 + maxValue = math.MaxInt16 case types.Int4: - opts.MaxValue = math.MaxInt32 - *upperIntBound = math.MaxInt32 - *lowerIntBound = math.MinInt32 + maxValue = math.MaxInt32 case types.Int: // Do nothing, it's the default. default: - return pgerror.Newf(pgcode.InvalidParameterValue, - "CREATE SEQUENCE option AS received type %s, must be integer", integerType) + return pgerror.Newf( + pgcode.InvalidParameterValue, + "CREATE SEQUENCE option AS received type %s, must be integer", + integerType, + ) } } else { - opts.MaxValue = -1 + maxValue = -1 switch integerType { case types.Int2: - opts.MinValue = math.MinInt16 - *upperIntBound = math.MaxInt16 - *lowerIntBound = math.MinInt16 + minValue = math.MinInt16 case types.Int4: - opts.MinValue = math.MinInt32 - *upperIntBound = math.MaxInt32 - *lowerIntBound = math.MinInt32 + minValue = math.MinInt32 case types.Int: // Do nothing, it's the default. default: - return pgerror.Newf(pgcode.InvalidParameterValue, - "CREATE SEQUENCE option AS received type %s, must be integer", integerType) + return pgerror.Newf( + pgcode.InvalidParameterValue, + "CREATE SEQUENCE option AS received type %s, must be integer", + integerType, + ) } } + if setMinValue { + opts.MinValue = minValue + } + if setMaxValue { + opts.MaxValue = maxValue + } return nil } @@ -421,13 +444,13 @@ func assignSequenceOptions( params *runParams, sequenceID descpb.ID, sequenceParentID descpb.ID, + existingType *types.T, ) error { + wasAscending := opts.Increment > 0 + // Set the default integer type of a sequence. var integerType = types.Int - var upperIntBound = int64(math.MaxInt64) - var lowerIntBound = int64(math.MinInt64) - // All other defaults are dependent on the value of increment // and the AS integerType. (i.e. whether the sequence is ascending // or descending, bigint vs. smallint) @@ -460,9 +483,36 @@ func assignSequenceOptions( opts.CacheSize = 1 } + lowerIntBound, upperIntBound, err := getSequenceIntegerBounds(integerType) + if err != nil { + return err + } + // Set default MINVALUE and MAXVALUE if AS option value for integer type is specified. if opts.AsIntegerType != "" { - if err := setSequenceIntegerBounds(opts, integerType, isAscending, &lowerIntBound, &upperIntBound); err != nil { + // 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 + } + } + + if err := setSequenceIntegerBounds( + opts, + integerType, + isAscending, + setMinValue, + setMaxValue, + ); err != nil { return err } } @@ -496,21 +546,11 @@ func assignSequenceOptions( // A value of nil represents the user explicitly saying `NO MINVALUE`. if option.IntVal != nil { opts.MinValue = *option.IntVal - if *option.IntVal < lowerIntBound { - return pgerror.Newf(pgcode.InvalidParameterValue, - "MINVALUE (%d) must be greater than (%d) for type %s", - *option.IntVal, lowerIntBound, integerType.SQLString()) - } } case tree.SeqOptMaxValue: // A value of nil represents the user explicitly saying `NO MAXVALUE`. if option.IntVal != nil { opts.MaxValue = *option.IntVal - if *option.IntVal > upperIntBound { - return pgerror.Newf(pgcode.InvalidParameterValue, - "MAXVALUE (%d) must be less than (%d) for type %s", - *option.IntVal, upperIntBound, integerType.SQLString()) - } } case tree.SeqOptStart: opts.Start = *option.IntVal @@ -559,25 +599,71 @@ func assignSequenceOptions( } } - // If start option not specified, set it to MinValue (for ascending sequences) - // or MaxValue (for descending sequences). - if _, startSeen := optionsSeen[tree.SeqOptStart]; !startSeen { - if opts.Increment > 0 { - opts.Start = opts.MinValue - } else { - opts.Start = opts.MaxValue + if setDefaults || (wasAscending && opts.Start == 1) || (!wasAscending && opts.Start == -1) { + // 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 + // ALTERed with the default original values. + if _, startSeen := optionsSeen[tree.SeqOptStart]; !startSeen { + if opts.Increment > 0 { + opts.Start = opts.MinValue + } else { + opts.Start = opts.MaxValue + } } } + if opts.MinValue < lowerIntBound { + return pgerror.Newf( + pgcode.InvalidParameterValue, + "MINVALUE (%d) must be greater than (%d) for type %s", + opts.MinValue, + lowerIntBound, + integerType.SQLString(), + ) + } + if opts.MaxValue < lowerIntBound { + return pgerror.Newf( + pgcode.InvalidParameterValue, + "MAXVALUE (%d) must be greater than (%d) for type %s", + opts.MaxValue, + lowerIntBound, + integerType.SQLString(), + ) + } + if opts.MinValue > upperIntBound { + return pgerror.Newf( + pgcode.InvalidParameterValue, + "MINVALUE (%d) must be less than (%d) for type %s", + opts.MinValue, + upperIntBound, + integerType.SQLString(), + ) + } + if opts.MaxValue > upperIntBound { + return pgerror.Newf( + pgcode.InvalidParameterValue, + "MAXVALUE (%d) must be less than (%d) for type %s", + opts.MaxValue, + upperIntBound, + integerType.SQLString(), + ) + } if opts.Start > opts.MaxValue { return pgerror.Newf( pgcode.InvalidParameterValue, - "START value (%d) cannot be greater than MAXVALUE (%d)", opts.Start, opts.MaxValue) + "START value (%d) cannot be greater than MAXVALUE (%d)", + opts.Start, + opts.MaxValue, + ) } if opts.Start < opts.MinValue { return pgerror.Newf( pgcode.InvalidParameterValue, - "START value (%d) cannot be less than MINVALUE (%d)", opts.Start, opts.MinValue) + "START value (%d) cannot be less than MINVALUE (%d)", + opts.Start, + opts.MinValue, + ) } return nil