sql: disallow table/view/sequence rename from making cross DB references#61741
sql: disallow table/view/sequence rename from making cross DB references#61741craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
c7ed2c3 to
5ed5961
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @fqazi)
pkg/sql/rename_table.go, line 190 at r1 (raw file):
} }
nit: this is a pretty big chunk of new logic. Any interest in reworking this into some functions? It'll also allow for some early returns which I suspect will aid readability.
One thing I lament is how hard it is to write unit tests for this stuff. The reason is pretty clear: constructing descriptors is awful. My sincere dream is that we'll get to a point where we can write SQL strings to build descriptors in memory and then unit test stuff like this. The fact that we go all the way to the logic tests pretty much always makes me
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/rename_table.go, line 190 at r1 (raw file):
Previously, ajwerner wrote…
nit: this is a pretty big chunk of new logic. Any interest in reworking this into some functions? It'll also allow for some early returns which I suspect will aid readability.
One thing I lament is how hard it is to write unit tests for this stuff. The reason is pretty clear: constructing descriptors is awful. My sincere dream is that we'll get to a point where we can write SQL strings to build descriptors in memory and then unit test stuff like this. The fact that we go all the way to the logic tests pretty much always makes me
sad, I was missing the word sad.
fqazi
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/rename_table.go, line 190 at r1 (raw file):
Previously, ajwerner wrote…
sad, I was missing the word sad.
Let me rework this into different functions, it would aid with readability.
d691fad to
e9c4ce4
Compare
fqazi
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/rename_table, line 466 at r1 (raw file):
Previously, postamar (Marius Posta) wrote…
This comment applies to all the lines added above: can you please conform the style of your SQL statements & queries to the existing standard? The existing standard is defined by https://sqlfum.pt/ which I'm told was built using the CRDB SQL parser.
Done.
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status:
pkg/sql/rename_table.go, line 325 at r2 (raw file):
Required: true,
I think this should skip the cache. Generally in the context of schema changes we want a consistent view of descriptors.
pkg/sql/rename_table.go, line 429 at r2 (raw file):
allDescriptors, err := p.Descriptors().GetAllDescriptors(ctx, p.txn)
This feels egregiously expensive. I'm pretty certain we track the backreference on the columns. See descpb.ColumnDescriptor.OwnsSequenceIDs
Fixes: cockroachdb#55709 Previously, disallowed cross DB references could be created using a ALTER TABLE/VIEW/SEQUENCE rename operations. This was inadequate because users could accidentally start using deprecated functionality. To address this, this patch detects such scenarios are returns an appropriate error. Release note (bug fix): ALTER TABLE/VIEW/SEQUENCE can no longer be used to incorrectly create cross DB references. Release justification: Low risk fix to avoid users from accidentally, using deprecated functionality.
fqazi
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/rename_table.go, line 325 at r2 (raw file):
Previously, ajwerner wrote…
Required: true,I think this should skip the cache. Generally in the context of schema changes we want a consistent view of descriptors.
Done.
pkg/sql/rename_table.go, line 429 at r2 (raw file):
Previously, ajwerner wrote…
allDescriptors, err := p.Descriptors().GetAllDescriptors(ctx, p.txn)This feels egregiously expensive. I'm pretty certain we track the backreference on the columns. See
descpb.ColumnDescriptor.OwnsSequenceIDs
Done
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:
|
bors r+ |
|
Build succeeded: |
Fixes: #55709
Previously, disallowed cross DB references could be created
using a ALTER TABLE/VIEW/SEQUENCE rename operations. This
was inadequate because users could accidentally start using
deprecated functionality. To address this, this patch detects
such scenarios are returns an appropriate error.
Release note (bug fix): ALTER TABLE/VIEW/SEQUENCE can no longer
be used to incorrectly create cross DB references.