sql: add sequence option info for identity columns under information_schema

[ZhouXing19]

Previously, for a column created with the GENERATED ... AS IDENTITY (seq_options) syntax, the info for the sequence option is not saved in the
information schema.

This commit is to fix it. We parse the sequence options saved as a string in the
the descriptor, so that it's much easier to extract specific options such as
sequence's start value or increment size.

To make sure that we get the same sequence option to generate
the sequence, we reuse assignSequenceOptions() by breaking it into several
helper functions.

fixes #82064
fixes #83208

Release note (sql): add sequence option info for identity columns under
information_schema

[cockroach-teamcity]

This change is 

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)

---

pkg/sql/information_schema.go line 481 at r1 (raw file):

				}

				var identityStart, identityIncrement, identityMax, identityMin tree.DString

nit: the type here can be *tree.DString

---

pkg/sql/information_schema.go line 484 at r1 (raw file):

				generatedAsIdentitySeqOpt := column.GetGeneratedAsIdentitySequenceOption()
				if generatedAsIdentitySeqOpt != nil {
					identityStart = *tree.NewDString(strconv.FormatInt(generatedAsIdentitySeqOpt.Start, 10))

nit: with the above comment, we won't need to dereference everything with *, which will avoid having to copy the string contents.

---

pkg/sql/catalog/tabledesc/column.go line 252 at r1 (raw file):

// GetGeneratedAsIdentitySequenceOption returns the column's `GENERATED AS
// IDENTITY` sequence option if it exists, empty string otherwise.
func (w column) GetGeneratedAsIdentitySequenceOption() *descpb.TableDescriptor_SequenceOpts {

nit: this should return an error as a second argument, and also the comment needs to be updated

also i wonder if the planner needs to be passed in here

---

pkg/sql/catalog/tabledesc/column.go line 258 at r1 (raw file):

	seqOpts, err := transformStringToSeqOption(*w.desc.GeneratedAsIdentitySequenceOption)
	if err != nil {
		return nil

nit: this should return err

---

pkg/sql/catalog/tabledesc/column.go line 267 at r1 (raw file):

// We don't do extra check for the values following the keywords, since they
// should have been checked via sql.go.
func transformStringToSeqOption(s string) (*descpb.TableDescriptor_SequenceOpts, error) {

nit: perhaps this can be named parseSequenceOpts

---

pkg/sql/catalog/tabledesc/column.go line 271 at r1 (raw file):

	arr := strings.Fields(strings.ToUpper(s))
	i := 0
	for i < len(arr) {

instead of this, i wonder if we can re-use the original SQL parser.

what if we do something like parser.ParseOne("CREATE SEQUENCE fake_name " + s)

then use a type assertion to turn the result into *tree.CreateSequence

then we can use the existing assignSequenceOptions function.

overall, it seems like that would be a less invasive change

---

pkg/sql/types/types.go line 2792 at r1 (raw file):

	case "int2", "smallint":
		return Int2
	case "int4", "integer":

i don't think this is correct if default_int_size is set to 8 (which is the default)

---

pkg/sql/types/types.go line 2794 at r1 (raw file):

	case "int4", "integer":
		return Int4
	case "int8", "bigint", "int64":

nit: i don't think int64 is valid here

---

pkg/sql/types/types.go line 2797 at r1 (raw file):

		return Int
	default:
		panic(fmt.Sprintf("unknown string %s for conversion to integet type", s))

nit: integer is misspelled

[ZhouXing19]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)

---

pkg/sql/information_schema.go line 481 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: the type here can be *tree.DString

I don't think so, seems that it will cause the nil pointer dereference if generatedAsIdentitySeqOpt == nil.

---

pkg/sql/catalog/tabledesc/column.go line 252 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: this should return an error as a second argument, and also the comment needs to be updated

also i wonder if the planner needs to be passed in here

Updated the returned types and the comments.
I don't think we need planner here. The planner is only used to get the sequence owner. We don't need this info in the information schema table.

---

pkg/sql/catalog/tabledesc/column.go line 258 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: this should return err

Done.

---

pkg/sql/catalog/tabledesc/column.go line 267 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: perhaps this can be named parseSequenceOpts

Done.

---

pkg/sql/catalog/tabledesc/column.go line 271 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

instead of this, i wonder if we can re-use the original SQL parser.

what if we do something like parser.ParseOne("CREATE SEQUENCE fake_name " + s)

then use a type assertion to turn the result into *tree.CreateSequence

then we can use the existing assignSequenceOptions function.

overall, it seems like that would be a less invasive change

Yup good idea, changed it to parser.ParseOne. I incline to use the new AssignSequenceOptions function here because we don't need owner info, so no need to pass in the planner.

---

pkg/sql/types/types.go line 2792 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i don't think this is correct if default_int_size is set to 8 (which is the default)

Deleted the whole function since we're using parse.ParseOne() now.

[rafiss]

just had small nits

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)

---

pkg/sql/information_schema.go line 481 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

I don't think so, seems that it will cause the nil pointer dereference if generatedAsIdentitySeqOpt == nil.

hm, actually maybe this needs to be different. i think right now, this will cause these columns to default to the empty string, but they actually should default to DNull

so initialize them as

				identityStart := tree.DNull
				identityIncrement := tree.DNull
				identityMax := tree.DNull
				identityMin := tree.DNull

then you won't need the copy with *tree.NewDString and & below

---

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

	}
	if err := tabledesc.AssignSequenceOptions(opts, optsNode, setDefaults, existingType); err != nil {
		return pgerror.Wrap(err, pgcode.InvalidParameterValue, "")

nit: you shouldn't wrap with an empty string. so either use pgerror.WithCandidateCode, or populate the error string passed into pgerror.Wrap

---

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

	}
	if err := assignSequenceOwner(ctx, p, opts, optsNode, sequenceID, sequenceParentID); err != nil {
		return pgerror.Wrap(err, pgcode.InvalidParameterValue, "")

ditto

---

pkg/sql/catalog/table_elements.go line 376 at r2 (raw file):

	// GetGeneratedAsIdentitySequenceOption returns the column's `GENERATED AS
	// IDENTITY` sequence option if it exists, empty string otherwise.

nit it should say "or nil otherwise"

---

pkg/sql/catalog/tabledesc/column.go line 271 at r2 (raw file):

// Note that this function is used to acquire the sequence option for the
// information schema table, so it doesn't parse for the sequence owner info.
func parseSequenceOpts(s string) (*descpb.TableDescriptor_SequenceOpts, error) {

i wonder if this parsing logic should live in the schemaexpr package instead. there could be a new sequence_options.go file there. what do you think makes sense?

---

pkg/sql/catalog/tabledesc/column.go line 583 at r2 (raw file):

	// Set the default integer type of a sequence.
	var integerType = types.Int

i wonder if this should be respecting default_int_size... well it looks like it wasn't before, so could you just leave a TODO (sql-exp) comment saying that we should decide if we want to make this use default_int_size

[ZhouXing19]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

---

pkg/sql/information_schema.go line 481 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

hm, actually maybe this needs to be different. i think right now, this will cause these columns to default to the empty string, but they actually should default to DNull

so initialize them as

				identityStart := tree.DNull
				identityIncrement := tree.DNull
				identityMax := tree.DNull
				identityMin := tree.DNull

then you won't need the copy with *tree.NewDString and & below

Ah right, thanks for the explanation! Done.

---

pkg/sql/information_schema.go line 484 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: with the above comment, we won't need to dereference everything with *, which will avoid having to copy the string contents.

Done.

---

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

Previously, rafiss (Rafi Shamim) wrote…

nit: you shouldn't wrap with an empty string. so either use pgerror.WithCandidateCode, or populate the error string passed into pgerror.Wrap

Done.

---

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

Previously, rafiss (Rafi Shamim) wrote…

ditto

Done.

---

pkg/sql/catalog/table_elements.go line 376 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit it should say "or nil otherwise"

Updated the comment.

---

pkg/sql/catalog/tabledesc/column.go line 271 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i wonder if this parsing logic should live in the schemaexpr package instead. there could be a new sequence_options.go file there. what do you think makes sense?

Yeah I agree it makes more sense to move the parsing logic to schemaexpr. Also moved AssignSequenceOptions in this new sequence_options.go as well, to avoid dependency loop.

---

pkg/sql/catalog/tabledesc/column.go line 583 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i wonder if this should be respecting default_int_size... well it looks like it wasn't before, so could you just leave a TODO (sql-exp) comment saying that we should decide if we want to make this use default_int_size

Yeah, nice catch! I also noticed this may be a bug for general sequences as well, so I made the issue #84554 to track. Also made a PR to fix it: #84555.

[rafiss]

nice work!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

---

pkg/sql/catalog/tabledesc/column.go line 583 at r2 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Yeah, nice catch! I also noticed this may be a bug for general sequences as well, so I made the issue #84554 to track. Also made a PR to fix it: #84555.

nice!

[ZhouXing19]

TFTR!
bors r=rafiss

[craig]

Build succeeded:

• GitHub CI (Cockroach)