sql: add ALTER TABLE ... SET/ADD IDENTITY to the declarative schema…#157144
sql: add ALTER TABLE ... SET/ADD IDENTITY to the declarative schema…#157144craig[bot] merged 2 commits intocockroachdb:masterfrom
ALTER TABLE ... SET/ADD IDENTITY to the declarative schema…#157144Conversation
|
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 |
pkg/sql/schemachanger/screl/attr.go
Outdated
| rel.EntityAttr(DescID, "TableID"), | ||
| rel.EntityAttr(ColumnID, "ColumnID"), | ||
| rel.EntityAttr(GeneratedAsIdentityType, "Type"), | ||
| rel.EntityAttr(GeneratedAsIdentitySequenceOption, "SequenceOption"), |
There was a problem hiding this comment.
Lets use the Expr attribute type here to have uniqueness. That will at least cut back on the changes needed to support more attributes.
|
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. |
fqazi
left a comment
There was a problem hiding this comment.
@shghasemi Great work, a few minor comments
@fqazi reviewed 3 of 43 files at r1, 74 of 74 files at r2, 53 of 53 files at r3, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @shghasemi)
pkg/sql/schemachanger/screl/attr.go line 58 at r2 (raw file):
// ReferencedDescID is the descriptor ID to which this element refers. ReferencedDescID // A string value, including:
Start with with the name of the field
// Value is a string value, for example:
...
Also lets add a Note here:
Fields referred to the Value attribute cannot be used in rules, and only used as a park of the key to make an element unique.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_add_identity.go line 35 at r3 (raw file):
colTypeElem := mustRetrieveColumnTypeElem(b, tbl.TableID, columnID) // Ensure that column is an integer if colTypeElem.Type == nil || colTypeElem.Type.InternalType.Family != types.IntFamily {
Is there any scenario where the type would be nil?
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_add_identity.go line 102 at r3 (raw file):
colDef, expr := alterTableAddColumnSerialOrGeneratedIdentity(b, colDef, tn) // 2. add the sequence owner b.Add(&scpb.SequenceOwner{
If there is some commonality between the add column we could break this ould into a function.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_add_identity.go line 103 at r3 (raw file):
// 2. add the sequence owner b.Add(&scpb.SequenceOwner{ SequenceID: expr.UsesSequenceIDs[0],
Nit: Paranoid me wants to assert that this is always 1.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_add_identity.go line 113 at r3 (raw file):
Expression: *expr, }) // ?? b.IncrementSchemaChangeAddColumnQualificationCounter("default_expr")
These are limited only to new columns to not needed.
This change renames `Comment` to `Value` to be used as a generic string type in other schema changer elements. This attr is not used to filter schema change element, and can be used as a generic type to avoid adding new attrs. Epic: CRDB-31283 Release note: None
Potential Bug(s) DetectedThe three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation. Next Steps: Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary. After you review the findings, please tag the issue as follows:
|
Done
No, you are correct. Fixed.
Most of the logic is shared. I made a small refactor to separate the create sequence functionality.
Done
Removed. |
fqazi
left a comment
There was a problem hiding this comment.
@fqazi reviewed 41 of 41 files at r4, 41 of 41 files at r5, all commit messages.
Reviewable status:
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 405 at r5 (raw file):
} func alterTableCreateSequence(
Maybe somerthing like: alterTableCreateColumnSequence
… changer This change adds support for `ALTER TABLE ... SET/ADD IDENTITY` to the declarative schema changer. This schema change operation runs using the legacy schema changer in versions older than 26.1. Epic: CRDB-31283 Fixes cockroachdb#142918 Release note (sql change): `ALTER TABLE ... SET/ADD GENERATED AS IDENTITY` is supported by the declerative schema changer in 26.1 or later.
Done |
|
TFTR! bors r+ |
155872: changefeedccl: fix timestamp for desc fetches during planning r=andyyang890,aerfrei a=log-head When a changefeed is planned and executed, there are several places where target table descriptors are fetched. With a db-level changefeed, the set of tables can change during changefeed startup. Now, the timestamp is set to the schema timestamp for retrieving table descriptors. Adding a schema_ts to the protobuf for ChangeAggregatorSpec and ChangeFrontierSpec. Fixes: #154549 Epic: CRDB-1421 Release note: None 157144: sql: add `ALTER TABLE ... SET/ADD IDENTITY` to the declarative schema… r=shghasemi a=shghasemi … changer This change adds support for `ALTER TABLE ... SET/ADD IDENTITY` to the declarative schema changer. This schema change operation runs using the legacy schema changer in versions older than 26.1. Epic: CRDB-31283 Fixes #142918 Release note (sql change): `ALTER TABLE ... SET/ADD GENERATED AS IDENTITY` is supported by the declerative schema changer in 26.1 or later. 157158: roachtest: do not build arm64 and fips if probabilitiy is 0 r=DarrylWong a=rail Previously, even when the probabilities for arm64 and fips builds were set to 0, the nightly roachtest script would still build those variants, which is unnecessary. This change modifies the script to skip building arm64 and fips variants when their respective probabilities are set to 0. Epic: none Release note: none Co-authored-by: Matthew Lougheed <matthew.lougheed@cockroachlabs.com> Co-authored-by: Shadi Ghasemitaheri <shadi.ghasemitaheri@cockroachlabs.com> Co-authored-by: Rail Aliiev <rail@iqchoice.com>
|
Build failed (retrying...): |
155872: changefeedccl: fix timestamp for desc fetches during planning r=andyyang890,aerfrei a=log-head When a changefeed is planned and executed, there are several places where target table descriptors are fetched. With a db-level changefeed, the set of tables can change during changefeed startup. Now, the timestamp is set to the schema timestamp for retrieving table descriptors. Adding a schema_ts to the protobuf for ChangeAggregatorSpec and ChangeFrontierSpec. Fixes: #154549 Epic: CRDB-1421 Release note: None 157144: sql: add `ALTER TABLE ... SET/ADD IDENTITY` to the declarative schema… r=shghasemi a=shghasemi … changer This change adds support for `ALTER TABLE ... SET/ADD IDENTITY` to the declarative schema changer. This schema change operation runs using the legacy schema changer in versions older than 26.1. Epic: CRDB-31283 Fixes #142918 Release note (sql change): `ALTER TABLE ... SET/ADD GENERATED AS IDENTITY` is supported by the declerative schema changer in 26.1 or later. Co-authored-by: Matthew Lougheed <matthew.lougheed@cockroachlabs.com> Co-authored-by: Shadi Ghasemitaheri <shadi.ghasemitaheri@cockroachlabs.com>
|
This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried. |
… changer
This change adds support for
ALTER TABLE ... SET/ADD IDENTITYto the declarative schema changer. This schema change operation runs using the legacy schema changer in versions older than 26.1.Epic: CRDB-31283
Fixes #142918
Release note (sql change):
ALTER TABLE ... SET/ADD GENERATED AS IDENTITYis supported by the declerative schema changer in 26.1 or later.