sql: Added CREATE SEQUENCE ... AS <typename> option.

[mokaixu]

Fixes: #25110
Fixes: #29046

Previously, unimplementedWithIssueDetail error was thrown
when an AS option in CREATE SEQUENCE ... AS was encountered.

Now we support the AS option for creating sequences which can
take on values such as smallint/integer/bigint. Typically (i.e.
in Postgres) the AS option allows users to specify an integer
type, which dictates the min and max integer values a sequence
can take on. Before this change, the only option value we supported was
bigint/int8/int/integer, which corresponds to our default
min and max values for sequences.

NB: In postgres, integer corresponds to int4. In cockroach, we use
nakedIntType which defaults to int8 but can be configured in the
cluster settings.

Release note (sql change): Added CREATE SEQUENCE AS option.

[cockroach-teamcity]

This change is 

[arulajmani]

Thanks for taking this on and putting it out so quick! 💯 Took an initial pass at this and left some preliminary thoughts, I can take a closer look once you "un-WIP" this.

Also, I think this PR could benefit from some logictests. I would also recommend prefixing the issue numbers linked above with a # in your commit message -- that way, the PR gets linked to the issues and they'll get automatically closed when this merges.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, and @mokaixu)

---

pkg/sql/sequence.go, line 279 at r1 (raw file):

		switch option.Name {
		case tree.SeqOptAs:

How would this work when explicit values for Min/Max are supplied in the sequence options? Concretely, what happens when the MaxValue supplied is greater than the one allowed by the datatype?

---

pkg/sql/parser/sql.y, line 6489 at r1 (raw file):

sequence_option_elem:
  AS typename                  { $$.val = tree.SequenceOption{Name: tree.SeqOptAs, AsTypename: $2.colType()}}

As is, this will accept all possible types, but what we really want here is to constrain this to smalling, integer, and bigint. As this is a subset of numeric types, I would recommend creating a new rule for handling this.

[mokaixu]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @arulajmani)

---

pkg/sql/sequence.go, line 279 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

How would this work when explicit values for Min/Max are supplied in the sequence options? Concretely, what happens when the MaxValue supplied is greater than the one allowed by the datatype?

Would love a PM's insight on this, but I'm thinking the 2 approaches are

1. error out when the MAX VALUE set conflicts with the integer type configure
2. let the MAX VALUE override what the default max is for the integer type.

Since my change deals with int64 (which is the largest encompassing integer type), option 2) seems reasonable to me. If/when we want to implement support for int2/int4

---

pkg/sql/parser/sql.y, line 6489 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

As is, this will accept all possible types, but what we really want here is to constrain this to smalling, integer, and bigint. As this is a subset of numeric types, I would recommend creating a new rule for handling this.

dare i say i'm creating ARUL?!?!??!?!?!?

[arulajmani]

Thanks for making changes, this is getting there. I have concerns that as is, this breaks round trip ability of SHOW CREATE SEQUENCE. Also, as I mentioned before, could you write some logic tests for this? I think it would greatly simplify some of the tests you have going on here. It might be worth adding some for ALTER SEQUENCE as well, as this patch affects both but only seems to focus on CREATE in its testing.

Reviewed 5 of 14 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, and @mokaixu)

---

pkg/sql/sequence.go, line 279 at r1 (raw file):

Previously, mokaixu (Monica Xu) wrote…

Would love a PM's insight on this, but I'm thinking the 2 approaches are

1. error out when the MAX VALUE set conflicts with the integer type configure
2. let the MAX VALUE override what the default max is for the integer type.

Since my change deals with int64 (which is the largest encompassing integer type), option 2) seems reasonable to me. If/when we want to implement support for int2/int4

Your comment looks out of date, looking at the state of the code below. In general, we would want to match Postgres as closely as possible. Postgres is your PM ;)

---

pkg/sql/sequence.go, line 249 at r2 (raw file):

	// All other defaults are dependent on the value of increment,
	// i.e. whether the sequence is ascending or descending.

This comment could benefit from an update now.

---

pkg/sql/sequence.go, line 267 at r2 (raw file):

	if setDefaults {

		// TODO: Move this to a setDefaults helper function

Sounds like a good idea, are you planning on doing it as part of this PR? If not, I'd recommend adding someones name to the TODO. Maybe @adityamaru, considering you'll be leaving?

---

pkg/sql/sequence.go, line 278 at r2 (raw file):

			case types.Int2:
				opts.MaxValue = math.MaxInt16
				upperIntBound = math.MaxInt16

Should this be in the setDefaults if condition? Seems like you're using these bounds to check min/max value assigning later on. This would break the ALTER SEQUENCE ... MAX VALUE ... case, if the max value being set is out of bounds, right?

---

pkg/sql/sequence_test.go, line 412 at r2 (raw file):

}

func TestCreateSequenceAsOption(t *testing.T) {

There's already a SHOW CREATE SEQUENCE test that you've added to in show_test.go. I'm wondering if all these test cases can just move there instead? As for the errors you're checking here, those should probably make their way to logictests.

---

pkg/sql/show_test.go, line 475 at r2 (raw file):

		{
			`CREATE SEQUENCE %s AS bigint`,
			`CREATE SEQUENCE public.%s AS int MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 1`,

This breaks round trip ability of SHOW CREATE SEQUENCE when the default int size is set to INT4 -- see my comments below.

---

pkg/sql/catalog/descpb/structured.proto, line 962 at r2 (raw file):

    optional SequenceOwner sequence_owner = 6 [(gogoproto.nullable) = false];
    
    optional string as_integer_type = 7 [(gogoproto.nullable) = false];

nit: Could you add a comment here.

---

pkg/sql/parser/parse_test.go, line 1869 at r2 (raw file):

		// `int` and `integer` parse to nakedIntTyoe which by default is Int/int64.
		{`CREATE SEQUENCE a AS integer`, `CREATE SEQUENCE a AS int`},
		{`CREATE SEQUENCE a AS bigint`, `CREATE SEQUENCE a AS int`},

This breaks round trip ability of SHOW CREATE TABLE?. Consider, CREATE SEQUENCE AS BIGINT which resolves to CREATE SEQUENCE AS INT, but INT can resolve to INT4 based on the cluster/session setting.

---

pkg/sql/parser/sql.y, line 6489 at r1 (raw file):

Previously, mokaixu (Monica Xu) wrote…

dare i say i'm creating ARUL?!?!??!?!?!?

good one hehe 😂

---

pkg/sql/parser/sql.y, line 202 at r2 (raw file):

func (u *sqlSymUnion) colType2() *types.T {

This looks unused.

---

pkg/sql/parser/sql.y, line 6533 at r2 (raw file):

// %Text:
// CREATE [TEMPORARY | TEMP] SEQUENCE <seqname>
//   [AS <typename>]

Could you add this docs thing to ALTER SEQUENCE as well!

---

pkg/sql/parser/sql.y, line 9487 at r2 (raw file):

  }

nit: extra line

---

pkg/sql/sem/tree/create.go, line 1464 at r2 (raw file):

// SequenceOptAs represents possible values (integer types)
// the AS option of a CREATE SEQUENCE statement can have.
type SequenceOptAs int

Is there a meaningful use of this anymore? Looks like AsIntegerType below does exactly what this would've aimed to accomplish.

[mokaixu]

Some notes in the most recent change:

• The SQL parsing now will only permit: BIGINT,

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @arulajmani)

---

pkg/sql/sequence.go, line 279 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Your comment looks out of date, looking at the state of the code below. In general, we would want to match Postgres as closely as possible. Postgres is your PM ;)

Looks like Postgres errors out (approach #1)

See below

postgres=# CREATE SEQUENCE bad AS smallint MAXVALUE 200000;
ERROR:  MAXVALUE (200000) is out of range for sequence data type smallint

I've implemented that! but my message looks like this: "MAXVALUE (%d) must be less than (%d) for type %s",

---

pkg/sql/sequence.go, line 249 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

This comment could benefit from an update now.

done

---

pkg/sql/sequence.go, line 267 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Sounds like a good idea, are you planning on doing it as part of this PR? If not, I'd recommend adding someones name to the TODO. Maybe @adityamaru, considering you'll be leaving?

i just refactored it into a helper method.
Currently, I pass variables by reference and modify their value in the setSequenceDefaultVals method.

Another approach is to return the values of the variables. Which one do you think has more sensible design?

---

pkg/sql/sequence.go, line 278 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Should this be in the setDefaults if condition? Seems like you're using these bounds to check min/max value assigning later on. This would break the ALTER SEQUENCE ... MAX VALUE ... case, if the max value being set is out of bounds, right?

To clarify, setDefaults is used only for CREATE SEQUENCE? Otherwise, we are in an ALTER SEQUENCE?

If that is the case, then i will move this logic pertaining to integerType outside of the setDefaults if statement

---

pkg/sql/sequence_test.go, line 412 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

There's already a SHOW CREATE SEQUENCE test that you've added to in show_test.go. I'm wondering if all these test cases can just move there instead? As for the errors you're checking here, those should probably make their way to logictests.

Will look into writing logic tests

---

pkg/sql/show_test.go, line 475 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

This breaks round trip ability of SHOW CREATE SEQUENCE when the default int size is set to INT4 -- see my comments below.

Same response as below: how do we account for round trip ability when cluster settings are outside our control? Do we just want to hardcode

seq_opt_as_int:
  INT
  {
    $$.val = sqllex.(*lexer).nakedIntType
  }

I adopted this from the numeric SQL type which is the following:

// SQL numeric data types
numeric:
  INT
  {
    $$.val = sqllex.(*lexer).nakedIntType
  }
| INTEGER
  {
    $$.val = sqllex.(*lexer).nakedIntType
  }
| SMALLINT
  {
    $$.val = types.Int2
  }
| BIGINT
  {
    $$.val = types.Int
  }

---

pkg/sql/catalog/descpb/structured.proto, line 962 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: Could you add a comment here.

done

---

pkg/sql/parser/parse_test.go, line 1869 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

This breaks round trip ability of SHOW CREATE TABLE?. Consider, CREATE SEQUENCE AS BIGINT which resolves to CREATE SEQUENCE AS INT, but INT can resolve to INT4 based on the cluster/session setting.

I'm not sure how to fix this since Cluster/Session settings are the reason why this isn't round trippable (nakedIntType can be int4 or int8)

---

pkg/sql/parser/sql.y, line 6489 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

good one hehe 😂

🦃

---

pkg/sql/parser/sql.y, line 202 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

This looks unused.

oops. deleted

---

pkg/sql/parser/sql.y, line 6533 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Could you add this docs thing to ALTER SEQUENCE as well!

done

---

pkg/sql/parser/sql.y, line 9487 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: extra line

done

---

pkg/sql/sem/tree/create.go, line 1464 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Is there a meaningful use of this anymore? Looks like AsIntegerType below does exactly what this would've aimed to accomplish.

done

[arulajmani]

@mokaixu and I talked offline about this and how we can make SHOW CREATE SEQUENCE round-tripable. The tldr here is that BIGINT would resolve to INT8 instead of INT when formatting so that the statement round trips independent of the int size cluster setting. Currently. INT8 isn't accepted as a valid datatype value for CREATE SEQUENCE AS, but we confirmed that PG accepts it even though it doesn't explicitly state this in its docs. As part of making this roundtripable, we'll also accept INT8, INT4, etc. as possible values in CREATE SEQUENCE AS, to be in line w PG.

I can take another pass after we have the new approach implemented and more logictests -- thanks for iterating on this!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @mokaixu)

---

pkg/sql/sequence.go, line 279 at r1 (raw file):

Previously, mokaixu (Monica Xu) wrote…

Looks like Postgres errors out (approach #1)

See below

postgres=# CREATE SEQUENCE bad AS smallint MAXVALUE 200000;
ERROR:  MAXVALUE (200000) is out of range for sequence data type smallint

I've implemented that! but my message looks like this: "MAXVALUE (%d) must be less than (%d) for type %s",

minor nit would be to match the pg error message as closely as possible, but otherwise this looks good.

---

pkg/sql/sequence.go, line 278 at r2 (raw file):

Previously, mokaixu (Monica Xu) wrote…

To clarify, setDefaults is used only for CREATE SEQUENCE? Otherwise, we are in an ALTER SEQUENCE?

If that is the case, then i will move this logic pertaining to integerType outside of the setDefaults if statement

Yeah, exactly! ALTER SEQUENCE passes false to setDefaults above. And yes, what you're saying is exactly what I was alluding to, which is why I am encouraging you to write logictests for both create sequence and alter sequence with your new changes!

---

pkg/sql/sequence_test.go, line 412 at r2 (raw file):

Previously, mokaixu (Monica Xu) wrote…

Will look into writing logic tests

Okay, once you do, you might be able to come back here and clean this stuff up completely.

---

pkg/sql/show_test.go, line 475 at r2 (raw file):

Previously, mokaixu (Monica Xu) wrote…

Same response as below: how do we account for round trip ability when cluster settings are outside our control? Do we just want to hardcode

seq_opt_as_int:
  INT
  {
    $$.val = sqllex.(*lexer).nakedIntType
  }

I adopted this from the numeric SQL type which is the following:

// SQL numeric data types
numeric:
  INT
  {
    $$.val = sqllex.(*lexer).nakedIntType
  }
| INTEGER
  {
    $$.val = sqllex.(*lexer).nakedIntType
  }
| SMALLINT
  {
    $$.val = types.Int2
  }
| BIGINT
  {
    $$.val = types.Int
  }

No longer applicable after our discussion offline -- see above.

---

pkg/sql/parser/parse_test.go, line 1869 at r2 (raw file):

Previously, mokaixu (Monica Xu) wrote…

I'm not sure how to fix this since Cluster/Session settings are the reason why this isn't round trippable (nakedIntType can be int4 or int8)

not applicable anymore, see summary of discussion above.

[mokaixu]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @arulajmani)

---

pkg/sql/sequence.go, line 279 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

minor nit would be to match the pg error message as closely as possible, but otherwise this looks good.

I will address this as soon as CI passes!

---

pkg/sql/sequence.go, line 278 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Yeah, exactly! ALTER SEQUENCE passes false to setDefaults above. And yes, what you're saying is exactly what I was alluding to, which is why I am encouraging you to write logictests for both create sequence and alter sequence with your new changes!

doubly done!

---

pkg/sql/sequence_test.go, line 412 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Okay, once you do, you might be able to come back here and clean this stuff up completely.

I've deleted redundant tests in sequence_test.go compared to show_test.go . sequence_test.go now primarily tests basic sequence functions and possible errors that can come up in the business logic.

---

pkg/sql/show_test.go, line 475 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

No longer applicable after our discussion offline -- see above.

ack

---

pkg/sql/parser/parse_test.go, line 1869 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

not applicable anymore, see summary of discussion above.

ack

[arulajmani]

Yo! Sorry for taking so long to get back on reviewing this thing -- OOO and what not.

Took another look, and most of my comments now are around testing. Specifically, I'd feel much better about this change once ALTER SEQUENCE is tested more thoroughly as well. I've left some suggestions in comments about ways to do this. Other than that, I think this is close.

Reviewed 2 of 13 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, and @mokaixu)

---

pkg/sql/sequence_test.go, line 411 at r4 (raw file):

	require.NoError(t, err)
}

@adityamaru can these tests not move to logic tests? A lot of what seems to be tested here is also tested in the logic tests, but I'm not sure if every case is. I think it's worth removing this entire function here and moving all the interesting tests to logic tests.

---

pkg/sql/logictest/testdata/logic_test/sequences, line 1277 at r4 (raw file):

statement ok
CREATE SEQUENCE db2.seq2 OWNED BY db1.t.a

We don't seem to be doing anything interesting with these that isn't covered by parse tests, right?

---

pkg/sql/logictest/testdata/logic_test/sequences, line 1331 at r4 (raw file):

statement ok
ALTER SEQUENCE seqas_9 AS smallint

Would be nice to get more ALTER tests (maybe in the alter_sequence file?). In particular, I'm thinking of cases where altering to a larger/smaller Integer type would cause errors because of previously set MAXVALUE/MINVALUE/START etc.

Another interesting test idea would be to try ALTER-ing a sequence to a smaller Integer type when the curval is greater than the max permitted.

[otan]

i gotchu @mokaixu ! miss the memes.

i've updated the PR with arul's suggestions, the ALTER SEQUENCE bugs are fixed in the second commit. since arul's changed teams, maybe @rafiss can take a look.

[rafiss]

awesome! thanks for bringing this back

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, @mokaixu, and @otan)

---

pkg/sql/sequence_test.go, line 861 at r7 (raw file):

			verifyQuery: `SHOW CREATE SEQUENCE a_seq`,
			expected: [][]string{{
				"a_seq", `CREATE SEQUENCE public.a_seq AS INT8 MINVALUE 0 MAXVALUE 234567 INCREMENT 1 START 2`,

not sure -- from looking at most of these, it seems nay would work as logic tests. are there any that can't?

---

pkg/sql/catalog/descpb/structured.proto, line 1100 at r7 (raw file):

    // AS option value for CREATE SEQUENCE, which specifies the default
    // min and max values a sequence can take on.
    optional string as_integer_type = 7 [(gogoproto.nullable) = false];

hm this part seems backward incompatible. shouldn't as_integer_type be number 8?

[otan]

fixed up some more

Reviewed 1 of 13 files at r4, 4 of 17 files at r5, 1 of 1 files at r6, 3 of 6 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, @mokaixu, @otan, and @rafiss)

---

pkg/sql/sequence_test.go, line 412 at r2 (raw file):

Previously, mokaixu (Monica Xu) wrote…

I've deleted redundant tests in sequence_test.go compared to show_test.go . sequence_test.go now primarily tests basic sequence functions and possible errors that can come up in the business logic.

the logic tests look pretty exhaustive so i removed this.

---

pkg/sql/sequence_test.go, line 411 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

@adityamaru can these tests not move to logic tests? A lot of what seems to be tested here is also tested in the logic tests, but I'm not sure if every case is. I think it's worth removing this entire function here and moving all the interesting tests to logic tests.

I've done this.

---

pkg/sql/catalog/descpb/structured.proto, line 1100 at r7 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

hm this part seems backward incompatible. shouldn't as_integer_type be number 8?

i fixed this in a later revision - can confirm version-upgrade tests work!
bad rebase :(

[rafiss]

lgtm with one question

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, @mokaixu, @otan, and @rafiss)

---

pkg/sql/sequence.go, line 396 at r11 (raw file):

	if isAscending {
		minValue = 1

why does isAscending change the minValue? is that just a weird quirk of how PG works too?

[otan]

bors r=rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, @mokaixu, @otan, and @rafiss)

---

pkg/sql/sequence.go, line 396 at r11 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

why does isAscending change the minValue? is that just a weird quirk of how PG works too?

yep. 1 to max_int and -1 to -min_int is the increment styles.

[craig]

Merge conflict.

[otan]

bors r=rafiss

[craig]

Build succeeded:

• GitHub CI (Cockroach)