sql/schemachanger: support generated columns in ADD COLUMN#139135
sql/schemachanger: support generated columns in ADD COLUMN#139135craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
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. |
|
This change is |
rafiss
left a comment
There was a problem hiding this comment.
very nice that this was pretty simple to add, nice work. i just had one question.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @fqazi)
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 328 at r1 (raw file):
// Generate identity is always a SQL sequence based column. if isGeneratedColumn { serialNormalizationMode = sessiondatapb.SerialUsesSQLSequences
so i think this would lead to a divergence from current behavior. i wrote up a quick logic test to demonstrate the current behavior, where adding a column like this is GENERATED, but still backed by unique_rowid. can you add a test case like the one below? (and if we want to intentionally change the behavior, let's codify that in tests.)
statement ok
SET serial_normalization = rowid
statement ok
CREATE TABLE t(a INT PRIMARY KEY)
statement ok
ALTER TABLE t ADD COLUMN b SERIAL GENERATED ALWAYS AS IDENTITY
query T
SELECT crdb_internal.pb_to_json('desc', descriptor)->'table'->'columns'->1->>'defaultExpr'
FROM system.descriptor
WHERE id = 't_serial_identity_no_sequence'::regclass::oid
----
unique_rowid()
query T
SELECT crdb_internal.pb_to_json('desc', descriptor)->'table'->'columns'->1->>'generatedAsIdentityType'
FROM system.descriptor
WHERE id = 't_serial_identity_no_sequence'::regclass::oid
----
GENERATED_ALWAYS
spilchen
left a comment
There was a problem hiding this comment.
Do you think we need any tests added under sql/schemachanger/scbuild/testdata and sql/schemachanger/scplan/testdata? Or do you think we have sufficient coverage with the e2e tests you added.
Reviewed all commit messages.
Reviewable status:
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 328 at r1 (raw file):
// Generate identity is always a SQL sequence based column. if isGeneratedColumn { serialNormalizationMode = sessiondatapb.SerialUsesSQLSequences
Do we need to set this before we call telemetry.Inc(sqltelemetry.SerialColumnNormalizationCounter( above? It seems like we might increment the wrong counter instead.
Previously, the add column statement in the declarative schema changer only supported serial columns. This patch adds support for adding generated columns inside the declarative schema changer. Fixes: cockroachdb#128087 Release note: None
3f1db20 to
ac9a722
Compare
fqazi
left a comment
There was a problem hiding this comment.
Added extra tests for generated columns under both
Reviewable status:
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 328 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
so i think this would lead to a divergence from current behavior. i wrote up a quick logic test to demonstrate the current behavior, where adding a column like this is GENERATED, but still backed by unique_rowid. can you add a test case like the one below? (and if we want to intentionally change the behavior, let's codify that in tests.)
statement ok SET serial_normalization = rowid statement ok CREATE TABLE t(a INT PRIMARY KEY) statement ok ALTER TABLE t ADD COLUMN b SERIAL GENERATED ALWAYS AS IDENTITY query T SELECT crdb_internal.pb_to_json('desc', descriptor)->'table'->'columns'->1->>'defaultExpr' FROM system.descriptor WHERE id = 't_serial_identity_no_sequence'::regclass::oid ---- unique_rowid() query T SELECT crdb_internal.pb_to_json('desc', descriptor)->'table'->'columns'->1->>'generatedAsIdentityType' FROM system.descriptor WHERE id = 't_serial_identity_no_sequence'::regclass::oid ---- GENERATED_ALWAYS
Done.
It gets the same behaviour as before because IsGeneratedColumn is only set if the column is not serial. I added a test to confirm this
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 328 at r1 (raw file):
Previously, spilchen wrote…
Do we need to set this before we call
telemetry.Inc(sqltelemetry.SerialColumnNormalizationCounter(above? It seems like we might increment the wrong counter instead.
Done.
Good catch!
spilchen
left a comment
There was a problem hiding this comment.
Reviewed 20 of 26 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status:
rafiss
left a comment
There was a problem hiding this comment.
nice work! i added a backport label since the branch cut just happened today, but we want this in 25.1
Reviewable status:
Previously, the add column statement in the declarative schema changer only supported serial columns. This patch adds support for adding generated columns inside the declarative schema changer.
Fixes: #128087
Release note: None