sql/upgrades: add upgrade logic so that sequences are referenced by ID #81583
Conversation
|
This change is |
Xiang-Gu
force-pushed
the
migrate_sequence_to_be_referenced_by_id
branch
2 times, most recently
from
aa3c112 to
007e92e
Compare
Xiang-Gu
force-pushed
the
migrate_sequence_to_be_referenced_by_id
branch
from
007e92e to
798e769
Compare
There was a problem hiding this comment.
nice work on this, so sorry about the very slow review. Just nits
Reviewed 4 of 12 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @Xiang-Gu)
pkg/upgrade/tenant_upgrade.go line 47 at r1 (raw file):
TestingKnobs *TestingKnobs InternalPlannerConstructor func(txn *kv.Txn, descriptors *descs.Collection, currDb string) (interface{}, func())
can you narrow this to resolver.SchemaResolver for the return type?
pkg/upgrade/upgrades/upgrade_sequence_to_be_referenced_by_ID.go line 151 at r1 (raw file):
// Construct an internal planner planner, cleanup := d.InternalPlannerConstructor(txn, descriptors, dbDesc.GetName())
@chengxiong-ruan almost got the schemaResolver fully extracted from the planner. What do you think about finishing that work by extracting sql.schemaResolver into a new package and then constructing that thing directly? I think it'll be a good deal cleaner and will make me happy. Feel free to do it in follow-up.
Code quote:
,
Xiang-Gu
force-pushed
the
migrate_sequence_to_be_referenced_by_id
branch
from
798e769 to
82c14b3
Compare
There was a problem hiding this comment.
Thanks. I address the comments to change the return types. RFAL again.
I'm interested in finishing up @chengxiong-ruan's refactoring in a follow-up (also an exercise to learn more about the codebase) PR. I left a few questions in the comment below.
Reviewable status:
pkg/upgrade/tenant_upgrade.go line 47 at r1 (raw file):
Previously, ajwerner wrote…
can you narrow this to
resolver.SchemaResolverfor the return type?
done
pkg/upgrade/upgrades/upgrade_sequence_to_be_referenced_by_ID.go line 151 at r1 (raw file):
Previously, ajwerner wrote…
@chengxiong-ruan almost got the
schemaResolverfully extracted from the planner. What do you think about finishing that work by extractingsql.schemaResolverinto a new package and then constructing that thing directly? I think it'll be a good deal cleaner and will make me happy. Feel free to do it in follow-up.
I'd love to do that in a follow-up! Before that, can you explain a bit more about what needs to be done to finish that work?
I read @chengxiong-ruan's pr for his refactoring effort and learned that, previously, planner implemented the resolver.SchemaResolver interface, and Chengxiong moved them into a struct sql.schemaResolver and let this struct implement the resolver.SchemaResolver interface. He also left a TODO there saying "refactor this out into a separate package", which resonates with what you said. I don't, however, understand what's the "end state" we'd like to see to finish this work and what's required.
More concretely,
-
Where should this "separate" package be? Should it be the "resolver" package or somewhere else?
-
What do you mean by "constructing that thing directly"?
-
What is the "end-state" we'd like to see to finish this refactoring effort?
There was a problem hiding this comment.
Reviewed 4 of 12 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/upgrade/upgrades/upgrade_sequence_to_be_referenced_by_ID.go line 151 at r1 (raw file):
- Where should this "separate" package be? Should it be the "resolver" package or somewhere else?
It need to be somewhere else, like a schemaresolver package under sql package.
- What do you mean by "constructing that thing directly"?
It means you construct the SchemaResolver directly instead of the big mighty planner.
- What is the "end-state" we'd like to see to finish this refactoring effort?
The end state is that schemaResolver is not embedded within planner, instead, it's separate child field it depends on. So you see that planner itself won't be a SchemaResolver anymore. It'd be a happy lighter object.
Xiang-Gu
force-pushed
the
migrate_sequence_to_be_referenced_by_id
branch
from
82c14b3 to
dc96653
Compare
A few release ago, sequences are referenced by name but we have change it to be by ID (to enable things like `RENAME AS`). However, the necessary migration work, supposedly tracked in cockroachdb#61017, somehow fell out of the team's radar unfortunately. This PR filled this gap by implementing the migration logic, among many from v22.1 to v22.2, that iterates over all table (or view) descriptor and upgrade their sequence reference from by name to by ID, if not already. Release note: None
Xiang-Gu
force-pushed
the
migrate_sequence_to_be_referenced_by_id
branch
from
dc96653 to
796fa2c
Compare
|
bors r+ |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
A few releases ago, sequences are referenced by name but we have
changed it to be by ID (to enable things like
RENAME AS). However,the necessary migration work, supposedly tracked in #61017, somehow
fell out of the team's radar unfortunately. This PR filled this gap by
implementing the migration logic, among many from v22.1 to v22.2, that
iterates over all table (or view) descriptor and upgrade their sequence
reference from by name to by ID, if not already.
fixed: #61017
Release note: None