sql: encode and decode sequence regclasses, and allow sequence renaming#59864
sql: encode and decode sequence regclasses, and allow sequence renaming#59864craig[bot] merged 4 commits intocockroachdb:masterfrom the-ericwang35:ericwang/encode_decode_rename
Conversation
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 1 of 21 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @the-ericwang35)
pkg/sql/catalog/schemaexpr/column.go, line 324 at r1 (raw file):
// Swap out this node to use the qualified table name for the sequence. return false, &tree.AnnotateTypeExpr{ Type: types.String,
I think we want this to say regclass.
the-ericwang35
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/sql/catalog/schemaexpr/column.go, line 324 at r1 (raw file):
Previously, ajwerner wrote…
I think we want this to say regclass.
Done, as discussed the formatting now looks something like 'test.public.seq':::STRING::REGCLASS.
ajwerner
left a comment
There was a problem hiding this comment.
Nice! Very close
Reviewed 6 of 21 files at r1, 2 of 4 files at r2, 8 of 16 files at r3, 14 of 18 files at r4, 9 of 9 files at r5.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @the-ericwang35)
pkg/ccl/backupccl/restore_planning.go, line 1052 at r5 (raw file):
return err } *expr = newExpr
you can just do it right here in the same iteration loop.
pkg/sql/rename_database.go, line 317 at r5 (raw file):
Quoted 5 lines of code…
// We only don't allow this if the database name is in there. // This is always the last argument. if tree.Name(parsedSeqName.Parts[parsedSeqName.NumParts-1]).Normalize() == tree.Name(dbName).Normalize() { return false, column.GetName(), nil }
This is pretty 🤦 - I'm inclined to remove it but let's do it as a follow up.
pkg/sql/sequence.go, line 607 at r5 (raw file):
var seqDesc *tabledesc.Mutable var err error
nit: lower the error checking out of this branch.
pkg/sql/sequence.go, line 649 at r5 (raw file):
} if len(seqIdentifiers) > 0 && byID {
This probably deserves some commentary explaining what's going on.
pkg/sql/catalog/descpb/structured.proto, line 972 at r5 (raw file):
repeated uint32 column_ids = 3 [(gogoproto.customname) = "ColumnIDs", (gogoproto.casttype) = "ColumnID"]; // Indicates whether the relation is referenced via its name or its ID.
Maybe a bit more commentary about what it means for it to be referenced by name or by id.
pkg/sql/catalog/schemaexpr/column.go, line 290 at r5 (raw file):
// replaceWithSequenceNames replaces occurrences of sequence regclasses in an expression with // the sequence's fully qualified names.
This deserves more commentary explaining what it actually does.
pkg/sql/catalog/schemaexpr/column.go, line 291 at r5 (raw file):
// replaceWithSequenceNames replaces occurrences of sequence regclasses in an expression with // the sequence's fully qualified names. func replaceWithSequenceNames(
There's nothing that makes this actually about sequences... If the expression were 8:::REGCLASS it'd end up here, right? I think that's find and good but means that we should make these function names more generic.
pkg/sql/schemachanger/scpb/scpb.proto, line 86 at r5 (raw file):
uint32 sequence_id = 3 [(gogoproto.customname) = "SequenceID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb.ID"]; Type type = 4; bool by_id = 5 [(gogoproto.customname) = "ByID"];
commentary please
pkg/util/sequence/sequence.go, line 127 at r5 (raw file):
Quoted 5 lines of code…
// ReplaceSequenceNamesWithIDs walks the given expression, and replaces // any sequence names in the expression by their IDs instead. // e.g. nextval('foo') => nextval(123::regclass) func ReplaceSequenceNamesWithIDs( defaultExpr tree.Expr, nameToID map[string]int64,
This feels very much like the sort of thing that could be easy to unit test. Honestly, so too could the other function in this package. I think it's worth it.
the-ericwang35
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)
pkg/ccl/backupccl/restore_planning.go, line 1052 at r5 (raw file):
Previously, ajwerner wrote…
you can just do it right here in the same iteration loop.
Done.
pkg/sql/sequence.go, line 607 at r5 (raw file):
Previously, ajwerner wrote…
nit: lower the error checking out of this branch.
I realized declaring err is not even needed so I just removed it.
pkg/sql/sequence.go, line 649 at r5 (raw file):
Previously, ajwerner wrote…
This probably deserves some commentary explaining what's going on.
Done.
pkg/sql/catalog/descpb/structured.proto, line 972 at r5 (raw file):
Previously, ajwerner wrote…
Maybe a bit more commentary about what it means for it to be referenced by name or by id.
Done.
pkg/sql/catalog/schemaexpr/column.go, line 290 at r5 (raw file):
Previously, ajwerner wrote…
This deserves more commentary explaining what it actually does.
Done.
pkg/sql/catalog/schemaexpr/column.go, line 291 at r5 (raw file):
Previously, ajwerner wrote…
// replaceWithSequenceNames replaces occurrences of sequence regclasses in an expression with // the sequence's fully qualified names. func replaceWithSequenceNames(There's nothing that makes this actually about sequences... If the expression were
8:::REGCLASSit'd end up here, right? I think that's find and good but means that we should make these function names more generic.
Yep that's a good point, renamed to replaceIDsWithFQNames
pkg/sql/schemachanger/scpb/scpb.proto, line 86 at r5 (raw file):
Previously, ajwerner wrote…
commentary please
Done.
pkg/util/sequence/sequence.go, line 127 at r5 (raw file):
Previously, ajwerner wrote…
// ReplaceSequenceNamesWithIDs walks the given expression, and replaces // any sequence names in the expression by their IDs instead. // e.g. nextval('foo') => nextval(123::regclass) func ReplaceSequenceNamesWithIDs( defaultExpr tree.Expr, nameToID map[string]int64,This feels very much like the sort of thing that could be easy to unit test. Honestly, so too could the other function in this package. I think it's worth it.
Yep, I added some basic tests for all of the functions in this file in a new test file sequence_test.go, I can add more later if needed.
ajwerner
left a comment
There was a problem hiding this comment.
Just the typo nit, then bors it!
Reviewed 1 of 31 files at r10, 4 of 16 files at r11, 9 of 18 files at r12, 20 of 20 files at r13.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)
pkg/sql/schemachanger/scpb/scpb.proto, line 86 at r5 (raw file):
Previously, the-ericwang35 (Eric Wang) wrote…
Done.
seqiemce
nit: start this with ByID
This patch builds upon #59396, where we overloaded sequence operators to accept a regclass. Now that sequence operators can accept a regclass, this patch starts using this change by storing sequences via their `ID::regclass`, updating back references to distinguish between references via names and IDs, and then transforming these IDs back to their fully qualified names for printing. This patch also starts allowing sequences that are referenced only via ID to be renamed. Lastly, tests were added to demonstrate all of this new functionality. Release note (sql change): encode and decode sequence regclasses, and allow renaming sequences
Before, we did not allow sequences to be renamed if they were depended on by, since the dependency used the sequence's name. Now that we've started to use IDs for references instead, we want to allow renaming of sequences that are only referenced by their ID. Release note (sql change): allow renaming of sequences referenced by ID
The previous commit allowed sequences referenced only by IDs to be renamed. This patch further adds to this by converting all sequence references in DEFAULT expressions to be referenced by ID, even if the expression is given a sequence name string. This means that now, all new sequences can be renamed. Release note (sql change): convert sequences referenced by name to ID, allow all renaming
This patch adds a version gate to this feature, so that all DEFAULT expressions created in the latest version will references sequences by ID. And, all new sequences created will be able to be renamed. However, existing sequences and expressions created in previous versions will still be referenced by name, and cannot be renamed. Release note (sql change): add version gate to sequence regclass feature
the-ericwang35
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)
pkg/sql/schemachanger/scpb/scpb.proto, line 86 at r5 (raw file):
Previously, ajwerner wrote…
seqiemce
nit: start this with
ByID
Wow that spelling was not even close, fixed.
|
TFTR! bors r+ |
|
Build succeeded: |
61439: sql: reference sequences used in views by their IDs r=ajwerner a=the-ericwang35 Followup to #59864, fixes #61205. Previously, a change was introduced to store sequences used in DEFAULT expressions to be referenced by their IDs instead of their names, to allow sequence renaming. In this patch, we want to do something similar by rewriting sequence references in views to be referenced by their IDs. This allows sequences used in views to be renamed. Another thing this PR does is change the output of sequences to look like `'s'::REGCLASS` rather than `'s':::STRING::REGCLASS`, since we're no longer type checking them (since it's unnecessary). Release note (sql change): reference sequences used in views by their IDs to allow these sequences to be renamed. 61682: kvserver: teach replicateQueue to replace dead/decommisioning non-voters r=aayushshah15 a=aayushshah15 The PR is very similar to #59403. Previously, non-voters on dead or decommissioning nodes would not get removed or replaced. This commit fixes this behaviour. Resolves #61626 Release justification: fixes limitation where dead/decommissioning non-voters would not be removed or replaced Release note: None 61967: kvserver: stop spuriously refusing non-voters in replicateQueue.shouldQueue() r=aayushshah15 a=aayushshah15 Before this commit, the `replicateQueue` would refuse to queue up a replica into the queue (in its `shouldQueue` method) for the rebalancing case unless it could verify that a voting replica needed to be rebalanced. This was an unfortunate oversight since it meant that unless there was a voting replica to be rebalanced, non-voters would not get rebalanced by the queue. This commit fixes this bug. Noticed while debugging a flakey test for #61682 Release justification: bug fix Release note: None 62009: build: perform `bazel clean --expunge` on `make clean` r=rickystewart a=rickystewart Release note: None 62186: sql: ensure user has correct privileges when adding/removing regions r=richardjcai a=arulajmani Previously we did not account for privileges on database objects when adding the default locality config on first region add or removing the locality config on last region drop properly. In particular, we weren't adding/removing the locality config on any descriptor that wasn't visible to the user. This is bad because our validation logic expects only and all objects in multi-region databases to have a valid locality config. This means future accesses to such descriptors would fail validation. The root of this problem was the API choice here, `ForEachTableDesc`, which filters out invisible descriptors. This patch instead switches to using `forEachTableInMultiRegionDatabase`. While here, instead of issuing separate requests for every table, I refactored this thing to issue a single batch request instead. Now that we view all the descriptors inside the database, unfiltered, we perform privilege checks on them before proceeding with the add/drop operation. In particular, the semantics are: - admin users are allowed to add/drop regions as they wish. - non admin-users require the CREATE privilege or must have ownership on all the objects inside the database. Closes #61003 Release note (sql change): `ALTER DATABASE .. SET PRIMARY REGION` now requires both CREATE and ZONECONFIG privilege on all objects inside the database when adding the first region to the database. Same for dropping the last region using `ALTER DATABASE ... DROP REGION`. Co-authored-by: Eric Wang <ericw@cockroachlabs.com> Co-authored-by: Aayush Shah <aayush.shah15@gmail.com> Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com> Co-authored-by: arulajmani <arulajmani@gmail.com>
This patch builds upon #59396,
where we overloaded sequence operators to accept a regclass.
Now that sequence operators can accept a regclass,
this patch starts using this change by storing sequences
via their
ID::regclass, updating back references to distinguishbetween references via names and IDs, and then transforming
these IDs back to their fully qualified names for printing.
This patch also starts allowing sequences that are referenced only
via ID to be renamed.
Lastly, tests were added to demonstrate all of this new functionality.
Release note (sql change): encode and decode sequence regclasses,
and allow renaming sequences