sql: assign sequence ownership created when create/alter table

[chengxiong-ruan]

Fixes #69298

Release note (bug fix): A sequence is created when a column is
defined as SERIAL type and the serial_normalization session
variable is set to sql_sequence. In this case, the sequence is
owned by the column and the table where the column exists. The
sequence should be dropped when the owner table/column is dropped,
which is the postgres behavior. However, cockroach never set
ownership information correctly but only dependecy relationship.
So the sequence stays even the owner table/column does not exist
anymore. This fix assigns correct ownership information to sequence
descriptor and column descriptor so that cockroach can align with
postgres behavior.

[cockroach-teamcity]

This change is 

[chengxiong-ruan]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/add_column.go, line 170 at r1 (raw file):

	// If the new column has a DEFAULT or an ON UPDATE expression that uses a
	// sequence, add references between its descriptor and this column descriptor.
	if err := cdd.ForEachTypedExpr(func(expr tree.TypedExpr) error {

This block is mostly moved from above.

---

pkg/sql/add_column.go, line 178 at r1 (raw file):

		}
		for _, changedSeqDesc := range changedSeqDescs {
			if colOwnedSeqDesc != nil && colOwnedSeqDesc.ID == changedSeqDesc.ID {

I think in theory colOwnedSeqDesc and changedSeqDesc should refer to a same instance, we can set ownership outside of this loop by using colOwnedSeqDesc. But still want to make sure I'm modifying the right copy here just in case the assumption is not true any more in the future..

---

pkg/sql/create_sequence.go, line 177 at r1 (raw file):

}

func createSequencesForSerialColumns(

This function is mostly a copy paste extraction from create_table.go below. Also narrow down the params dependency to a subset of its fields.

---

pkg/sql/temporary_schema.go, line 272 at r1 (raw file):

		databaseIDToTempSchemaID[uint32(desc.GetParentID())] = uint32(desc.GetParentSchemaID())

		// If a sequence is owned by a table column, it is dropped when the owner

this is covered by an existing test

---

pkg/sql/catalog/tabledesc/table_desc.go, line 176 at r1 (raw file):

// SetSequenceOwner adds sequence id to the sequence id list owned by a column
// and set ownership values of sequence options.
func (desc *Mutable) SetSequenceOwner(colName tree.Name, table *Mutable) error {

I'm always not proud of myself whenever creating a function with side effects... We may not need a function for this at all.

[ajwerner]

I'm good with this. We should probably backport it, at least the last release. Backporting to 21.1 has gotten somewhat painful.

Reviewed 6 of 15 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @chengxiong-ruan)

---

pkg/ccl/importccl/read_import_mysql.go, line 433 at r2 (raw file):

		seqDesc, err = sql.NewSequenceTableDesc(
			ctx,
			nil,

nit: comment the nil

---

pkg/ccl/importccl/read_import_pgdump.go, line 319 at r2 (raw file):

		desc, err := sql.NewSequenceTableDesc(
			ctx,
			nil,

nit: comment the nil

---

pkg/sql/add_column.go, line 178 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

I think in theory colOwnedSeqDesc and changedSeqDesc should refer to a same instance, we can set ownership outside of this loop by using colOwnedSeqDesc. But still want to make sure I'm modifying the right copy here just in case the assumption is not true any more in the future..

I hear you. The assumption should be true. It used to not be true, but we did work in 2020 to make it such that if you resolve the same descriptor mutably in the same transaction you get the same object. You can rely on that. That being said, this is fine. It'd be better if you added some explanatory commentary.

---

pkg/sql/testutils.go, line 77 at r2 (raw file):

		desc, err := NewSequenceTableDesc(
			ctx,
			nil,

nit: comment the nil

---

pkg/sql/catalog/tabledesc/table_desc.go, line 176 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

I'm always not proud of myself whenever creating a function with side effects... We may not need a function for this at all.

How would you feel about having this as a helper function in the sql package as opposed to as a method on the descriptor?

---

pkg/sql/logictest/testdata/logic_test/serial, line 225 at r2 (raw file):

----
serial  CREATE TABLE public.serial (
        a INT8 NOT NULL DEFAULT nextval('public.serial_a_seq1'::REGCLASS),

what in this change caused the naming changes here?

---

pkg/ccl/importccl/testutils_test.go, line 63 at r2 (raw file):

		desc, err := sql.NewSequenceTableDesc(
			ctx,
			nil,

nit: comment the nil

[chengxiong-ruan]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, and @chengxiong-ruan)

---

pkg/sql/catalog/tabledesc/table_desc.go, line 176 at r1 (raw file):

Previously, ajwerner wrote…

How would you feel about having this as a helper function in the sql package as opposed to as a method on the descriptor?

yeah, I'm happy with that. It's better to have a function acting the side effect than a method on an object.

---

pkg/sql/logictest/testdata/logic_test/serial, line 225 at r2 (raw file):

Previously, ajwerner wrote…

what in this change caused the naming changes here?

there's a test before this line already create sequence with name public.serial_a_seq, and we have a logic to find the first available sequence name by appending incremental integers to a default sequence name. so there is a sequence of sequence name like public.serial_a_seq1, public.serial_a_seq2, public.serial_a_seq3 if we create more.... with this change since the sequence from previous test is dropped and the names become available when creating this table, these names change :(

[ajwerner]

Reviewed 1 of 15 files at r1, 1 of 11 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, and @chengxiong-ruan)

---

pkg/ccl/importccl/read_import_pgdump.go, line 315 at r4 (raw file):

	for schemaAndTableName, seq := range createSeq {
		schema, err := getSchemaByNameFromMap(ctx, schemaAndTableName, schemaNameToDesc, execCfg.Settings.Version)
		if err != nil {

This is a good catch!

[chengxiong-ruan]

bors r+

[craig]

Build failed:

• GitHub CI (Cockroach)

[ajwerner]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 6afe54e to blathers/backport-release-21.2-74840: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.

---

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