sql: overload sequence operators to accept a regclass#59396
sql: overload sequence operators to accept a regclass#59396craig[bot] merged 1 commit intocockroachdb:masterfrom the-ericwang35:ericwang/overload_nextval
Conversation
|
This change is |
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @the-ericwang35)
pkg/sql/logictest/testdata/logic_test/sequences, line 62 at r1 (raw file):
query I SELECT nextval(53::regclass)
almost definitely a bad idea to rely on this number being stable. Instead consider 'lastval_test_2'::regclass. Same goes for all of these.
the-ericwang35
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/sequences, line 62 at r1 (raw file):
Previously, ajwerner wrote…
almost definitely a bad idea to rely on this number being stable. Instead consider
'lastval_test_2'::regclass. Same goes for all of these.
I've changed it to use let to query and store the IDs in a variable instead
ajwerner
left a comment
There was a problem hiding this comment.
needs a rebase but generally
Reviewed 1 of 7 files at r1, 1 of 2 files at r2, 2 of 2 files at r3.
Reviewable status:
pkg/sql/sequence.go, line 85 at r3 (raw file):
return 0, err } fmt.Printf("ID: %v", descriptor.ID)
remove
pkg/sql/sequence.go, line 213 at r3 (raw file):
return err } fmt.Printf("ID: %v", descriptor.ID)
remove
pkg/sql/logictest/testdata/logic_test/sequences, line 62 at r3 (raw file):
let $lastval_test_id SELECT id FROM system.namespace WHERE name='lastval_test'
This is fine but maybe SELECT 'lastval_test'::regclass::int is a bit better because this would be ambiguous if this sequence were in another database.
the-ericwang35
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sequence.go, line 85 at r3 (raw file):
Previously, ajwerner wrote…
remove
Whoops sorry, done.
pkg/sql/sequence.go, line 213 at r3 (raw file):
Previously, ajwerner wrote…
remove
Done.
pkg/sql/logictest/testdata/logic_test/sequences, line 62 at r3 (raw file):
Previously, ajwerner wrote…
This is fine but maybe
SELECT 'lastval_test'::regclass::intis a bit better because this would be ambiguous if this sequence were in another database.
Good point, done.
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 2 of 7 files at r1, 3 of 3 files at r4.
Reviewable status:
Previously, sequence operators (nextval, setval, currval) only accepted a string to refer to the referenced sequence. This patch allows these operators to accept a regclass as well. Tests were also added to verify this. This is the first step to fix this issue: #51090. Release note (sql change): overload sequence operators to accept a regclass
|
TFTR! bors r+ |
|
Build succeeded: |
59864: sql: encode and decode sequence regclasses, and allow sequence renaming r=the-ericwang35 a=the-ericwang35 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 Co-authored-by: Eric Wang <ericw@cockroachlabs.com>
sql: overload sequence operators to accept a regclass
Previously, sequence operators (nextval, setval, currval)
only accepted a string to refer to the referenced sequence.
This patch allows these operators to accept a regclass as well.
Tests were also added to verify this.
This is the first step to fix this issue:
#51090.
Release note (sql change): overload sequence operators to accept a regclass