sql: Allow Identity Column to Change Generation Type

[andrew-delph]

sql: Allow Alter Column to ADD and SET Identity
Informs: #110010

This change adds a new command, 'ALTER TABLE
ALTER [COLUMN] ADD GENERATED
{ ALWAYS | BY DEFAULT } [( <opt_sequence_option_list> )]'
and 'ALTER TABLE ALTER [COLUMN] SET
GENERATED { ALWAYS | BY DEFAULT }',
which allows a column to become an Identity
or change the generation type of the the
identity.

Release note (sql change): Columns can have
become an IdentityType by running 'ALTER
TABLE t ALTER COLUMN c ADD GENERATED ALWAYS
[( <opt_sequence_option_list> )]' or 'ALTER TABLE t ALTER
COLUMN c ADD GENERATED BY DEFAULT [( <opt_sequence_option_list> )]'.
Identity columns can have their IdentityType altered
by running 'ALTER TABLE t ALTER COLUMN c SET GENERATED ALWAYS'
or 'ALTER TABLE t ALTER COLUMN c SET GENERATED BY DEFAULT'.

[blathers-crl]

It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser.

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[fqazi]

This all looks good, just one minor comment for the commit message.

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrew-delph)

---

-- commits line 4 at r1:
Change this to Informs: #110010

[rafiss]

thanks for your contribution!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrew-delph)

---

pkg/sql/alter_table.go line 1064 at r1 (raw file):

	case *tree.AlterTableSetGeneratedAlways:
		if !col.IsGeneratedAsIdentity() {
			return sqlerrors.NewSyntaxErrorf("column %q is not an identity column", col.GetName())

nit: let's match the postgres error code, and use pgcode.ObjectNotInPrerequisiteState here:

return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,  
  "column %s of relation %s is not an identity column",  
  col.GetName(), tableDesc.GetName())

(and same for the one below)

---

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

Previously, andrew-delph (Andrew Delph) wrote…

Possibly, It would be better to return a struct here such as
AlterTableIdentity{
AlterTableIdentityOption option
}

It would give more similarity to Postgres and easier list options
https://github.com/postgres/postgres/blob/0e917508b89dd21c5bcd9183e77585f01055a20d/src/backend/parser/gram.y#L3037C7-L3037C7

i agree with this - let's use a struct like the one you described

---

pkg/sql/sem/tree/stmt.go line 2252 at r1 (raw file):

func (n *AlterTableDropNotNull) String() string               { return AsString(n) }
func (n *AlterTableDropStored) String() string                { return AsString(n) }
func (n *AlterTableSetGeneratedAlways) String() string        { return AsString(n) }

nit: should AlterTableSetGeneratedDefault be here too?

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[andrew-delph]

Thanks @fqazi and @rafiss for reviews.
I have updated the pr with the requested changes.

[rafiss]

thanks for the update! this is getting close

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrew-delph and @fqazi)

---

pkg/sql/parser/sql.y line 10184 at r3 (raw file):

  SET GENERATED_ALWAYS ALWAYS {
    x:= tree.GeneratedAlways
    $$.val = tree.IdentityOption{GeneratedAsIdentityType: &x}

as per the below comment, this should not be a pointer, so this should just be

$$.val = tree.IdentityOption{GeneratedAsIdentityType: tree.GeneratedAlways}

(and similar for below)

---

pkg/sql/sem/tree/alter_table.go line 746 at r3 (raw file):

type IdentityOption struct {
	SeqOption               *SequenceOption
	GeneratedAsIdentityType *GeneratedIdentityType

nit: this should not be a pointer. GeneratedIdentityType is an enum, so it should just be referenced by value.

[andrew-delph]

Hi @rafiss ,
I have modified the pr to include the 'ADD GENERATED...' command and also changed the parser layout.

Here is the current grammar:

This is what the change to the grammar looks like:

[andrew-delph]

• Possible a good reference for more tests can be found here: andrew-delph@b08b7cb#diff-fa3f37d689d1cc46fa50188c8b4b808e7f2b467e4424d45c47c72e57be8ff976
• Set gerenerated.... gramars can be cleaned
• functions used to add sequence can be cleaned possibly

[andrew-delph]

I have updated the set identity grammer to the following:

[andrew-delph]

Creating the column definition here in this way feels incorrect.
I believe that using the ColumnTableDef provides a way to use existing functions for generation of the identity sequence. The goal for doing this is to follow existing steps for the sequence creation.

After creation of the sequence, the existing column is then given ownership.

[andrew-delph]

Possibly, the tree.NewColumnTableDef can instead be called in the sql.y

[rafiss]

thanks for your contribution! i have updated the release note slightly, and merging this now

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[andrew-delph]

Thank you!