sql/schemachanger: support ADD COLUMN SERIAL for DSC unique_rowid only

[Dedej-Bergin]

Previously we could only add SERIAL columns with the legacy schema changer
with these code changes the ALTER TABLE ADD COLUMN statement
can now add SERIAL type columns via the declarative schema changer for
the default rowid serial_normalization mode.

Informs: #126900
Release note: none

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

[cockroach-teamcity]

This change is 

[fqazi]

Just some very minor comments. Great work!

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

---

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

						"upgrading the column %s to %s to utilize the session serial_normalization setting",
						d.Name.String(),
						types.Int.SQLString(),

Not sure why this change was made?

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 83 at r1 (raw file):

		}
		d = alterTableAddColumnSerial(b, d, tn)
		d.IsSerial = false

Why do we even need to flip this?

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 297 at r1 (raw file):

// UseFullWidthIntType For unique_rowid(), unordered_unique_rowid(), or virtual sequences,
// full-width integer type is used when the values will not fit otherwise.
func useFullWidthIntType(ctx BuildCtx, d *tree.ColumnTableDef, sender eval.ClientNoticeSender) {

Nit: Does this need to be a function? There is no-reuse here or clarity improvement.

---

pkg/sql/logictest/testdata/logic_test/alter_table line 3995 at r1 (raw file):

statement ok
SET serial_normalization = sql_sequence

Should we check other modes too?

[Dedej-Bergin]

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

---

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

Previously, fqazi (Faizan Qazi) wrote…

Not sure why this change was made?

It's leftovers from a failed refactoring attempt, thanks for catching it! Just updated it

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 83 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Why do we even need to flip this?

The d.IsSerial was already being flipped before, it was just happening a few function/method calls deep which made it a bit unclear.

But we need d.IsSerial to be false because it gets checked again lower in the code:

On line 115: cdd, err := tabledesc.MakeColumnDefDescs(b, d, b.SemaCtx(), b.EvalCtx(), tree.ColumnDefaultExprInAddColumn)

This MakeColumnDefDescs has this logic:

Code snippet:

func MakeColumnDefDescs(
	ctx context.Context,
	d *tree.ColumnTableDef,
	semaCtx *tree.SemaContext,
	evalCtx *eval.Context,
	defaultExprCtx tree.SchemaExprContext,
) (*ColumnDefDescs, error) {
	if d.IsSerial {
		// To the reader of this code: if control arrives here, this means
		// the caller has not suitably called processSerialInColumnDef()
		// prior to calling MakeColumnDefDescs. The dependent sequences
		// must be created, and the SERIAL type eliminated, prior to this
		// point.
		return nil, pgerror.New(pgcode.FeatureNotSupported,
			"SERIAL cannot be used in this context")
	}

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 297 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Nit: Does this need to be a function? There is no-reuse here or clarity improvement.

Thanks for catching this, it does not need to be a function, I just made the changes for this.

---

pkg/sql/logictest/testdata/logic_test/alter_table line 3995 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Should we check other modes too?

Yes, but maybe in future PRs? I'm planning on splitting this epic into 3 PRs, and I'll be re-writing/updating this testcase with the next PR as I will add support for sql_sequence. This is a temporary testcase, when we close this epic we won't need this specific testcase as sql_sequence and other modes will work and will not return an error.

[spilchen]

It looks good to me too. I had a refactor suggestion and some other tests that we may want.

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

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 80 at r1 (raw file):

	if d.IsSerial {
		if b.SessionData().SerialNormalizationMode != sessiondatapb.SerialUsesRowID {
			panic(scerrors.NotImplementedErrorf(d, "contains serial data type in unsupported mode"))

nit: I suggest moving this panic into alterTableAddColumnSerial. Additionally, consider adding a switch statement to handle the different modes, which would be a logical place for this change.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 286 at r1 (raw file):

		defType.Name(), b.SessionData().SerialNormalizationMode.String()))

	if defType.Width() < types.Int.Width() {

nit: I believe this logic is specific to using row ID as the default expression. And seeing as you need to expand this function for other serial normalization modes, could we structure this in a switch statement? That will make it easier to expand for other modes.

---

pkg/sql/logictest/testdata/logic_test/alter_table line 3971 at r1 (raw file):

subtest end

subtest alter_table_add_column_serial

Any tests to verify the dsc plan? i.e. in pkg/sql/schemachanger/scbuild/testdata, pkg/sql/schemachanger/scplan/testdata, and/or pkg/sql/schemachanger/testdata/end_to_end

---

pkg/sql/logictest/testdata/logic_test/alter_table line 3977 at r1 (raw file):

statement ok
create table roach (id int);

Can we add rows to the table? I think the alter is more interesting if it has to generate values for the new column.

[blathers-crl]

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

[Dedej-Bergin]

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

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 80 at r1 (raw file):

Previously, spilchen wrote…

nit: I suggest moving this panic into alterTableAddColumnSerial. Additionally, consider adding a switch statement to handle the different modes, which would be a logical place for this change.

Thanks Matt! I was thinking of moving the panic and adding the switch on my next PR as the code grows with additional mode support.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 286 at r1 (raw file):

Previously, spilchen wrote…

nit: I believe this logic is specific to using row ID as the default expression. And seeing as you need to expand this function for other serial normalization modes, could we structure this in a switch statement? That will make it easier to expand for other modes.

Thanks Matt! I'll add the switch on my next PR if that's okay? As the code grows with additional mode support.

---

pkg/sql/logictest/testdata/logic_test/alter_table line 3971 at r1 (raw file):

Previously, spilchen wrote…

Any tests to verify the dsc plan? i.e. in pkg/sql/schemachanger/scbuild/testdata, pkg/sql/schemachanger/scplan/testdata, and/or pkg/sql/schemachanger/testdata/end_to_end

Just added them! Thanks Matt!

---

pkg/sql/logictest/testdata/logic_test/alter_table line 3977 at r1 (raw file):

Previously, spilchen wrote…

Can we add rows to the table? I think the alter is more interesting if it has to generate values for the new column.

Sounds good, I added some rows, thanks Matt!

[spilchen]

Reviewed 2 of 2 files at r2, 1 of 1 files at r3, 15 of 15 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 80 at r1 (raw file):

Previously, Dedej-Bergin (Bergin Dedej) wrote…

Thanks Matt! I was thinking of moving the panic and adding the switch on my next PR as the code grows with additional mode support.

Yes, this is totally fine by me.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 83 at r1 (raw file):

Previously, Dedej-Bergin (Bergin Dedej) wrote…

The d.IsSerial was already being flipped before, it was just happening a few function/method calls deep which made it a bit unclear.

But we need d.IsSerial to be false because it gets checked again lower in the code:

On line 115: cdd, err := tabledesc.MakeColumnDefDescs(b, d, b.SemaCtx(), b.EvalCtx(), tree.ColumnDefaultExprInAddColumn)

This MakeColumnDefDescs has this logic:

I would suggest that we leave a comment so that we know why we flip it. Also, could we do the flip inside alterTableAddColumnSerial? I think that makes sense logically since we are gong to be returning a modified ColumnTableDef anyway.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 286 at r1 (raw file):

Previously, Dedej-Bergin (Bergin Dedej) wrote…

Thanks Matt! I'll add the switch on my next PR if that's okay? As the code grows with additional mode support.

Yes, I'm okay with this. Thanks.

[Dedej-Bergin]

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 83 at r1 (raw file):

Previously, spilchen wrote…

I would suggest that we leave a comment so that we know why we flip it. Also, could we do the flip inside alterTableAddColumnSerial? I think that makes sense logically since we are gong to be returning a modified ColumnTableDef anyway.

Sounds good, I added the comment and moved it to alterTableAddColumnSerial.

[spilchen]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Dedej-Bergin and @fqazi)

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 301 at r5 (raw file):

	}

	// Flip to false, as we validate or panic with this in MakeColumnDefDescs.

minor nit: Would it be better to phrase this in terms of what we're doing rather than what we're avoiding? For example:

// Serial is an alias for a real column definition. Set to false to indicate the alias has been remapped.

I also noticed that in the function comment for MakeColumnDefDescs, we specify which functions should be called to clear the IsSerial flag. Should we also mention alterTableAddColumnSerial for DSC alters?

[Dedej-Bergin]

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 301 at r5 (raw file):

Previously, spilchen wrote…

minor nit: Would it be better to phrase this in terms of what we're doing rather than what we're avoiding? For example:

// Serial is an alias for a real column definition. Set to false to indicate the alias has been remapped.

I also noticed that in the function comment for MakeColumnDefDescs, we specify which functions should be called to clear the IsSerial flag. Should we also mention alterTableAddColumnSerial for DSC alters?

Yes, I just changed the comment. For the function comment for MakeColumnDefDescs I was thinking of trying to refactor it in a later PR. So maybe for now I will leave it as is and I will either refactor it or update the comment for it in a future PR if that's okay?

[fqazi]

Reviewed 1 of 2 files at r2, 4 of 15 files at r4, 1 of 2 files at r5, 12 of 12 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @spilchen)

[Dedej-Bergin]

bors r+

[spilchen]

Reviewed all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @fqazi)

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 301 at r5 (raw file):

Previously, Dedej-Bergin (Bergin Dedej) wrote…

Yes, I just changed the comment. For the function comment for MakeColumnDefDescs I was thinking of trying to refactor it in a later PR. So maybe for now I will leave it as is and I will either refactor it or update the comment for it in a future PR if that's okay?

Yes, that's fine. Thanks for revising.

[celiala]

bors crashed yesterday. re-trying:

bors r+

[craig]

Build succeeded:

• check_generated_code
• macos_arm64_build
• local_roachtest
• linux_amd64_build
• linux_arm64_build
• local_roachtest_fips
• windows_build
• examples_orms
• linux_amd64_fips_build
• macos_amd64_build
• acceptance
• docker_image_amd64
• unit_tests
• lint