cli: add support for cockroach dump to operate on UDTs
#49808
Conversation
|
This change is |
|
Wanted to get a preliminary look at this PR while some open questions around cross database type references are resolved. The work done here is maybe another reason why it would be easier to do more of dump on the server side. |
pkg/cli/dump.go
Outdated
| @@ -169,6 +183,17 @@ func runDump(cmd *cobra.Command, args []string) error { | |||
| } | |||
| } | |||
|
|
|||
| if shouldDumpTypes && dumpCtx.dumpMode != dumpDataOnly { | |||
| if _, err := fmt.Fprintf(w, "SET experimental_enable_enums = true;\n"); err != nil { | |||
There was a problem hiding this comment.
For cross-version compatibility, be careful to tolerate dump talking to a server prior to your changes being available.
There was a problem hiding this comment.
(I have commented elsewhere, including on #28948, that the current architecture is flawed - all this logic should be done server-side, in which case the cross-version compat story is much easier.)
rohany
force-pushed
the
udt-dump
branch
4 times, most recently
from
a8336f8
to
5912c14
Compare
There was a problem hiding this comment.
Thanks for updating the PR but this is a bit out of my depth for a quality review right now.
As you hand the review over to another engineer, be mindful to check:
- that this code works fine when ran with a server running at a lower version
- that the tests exercise the various error cases
Reviewed 1 of 6 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @dt, @knz, and @rohany)
|
@dt can you take a look at this PR. Maybe @jordanlewis could take a look at the enum specific things. |
rohany
force-pushed
the
udt-dump
branch
2 times, most recently
from
342f299
to
5dd1cb5
Compare
|
Pinging for reviews! |
rohany
force-pushed
the
udt-dump
branch
2 times, most recently
from
5c7bdb5
to
cb15bb1
Compare
|
cc @mjibson for some small sqlsmith changes |
|
sqlsmith stuff LGTM |
There was a problem hiding this comment.
overall LGTM though like @knz I'd prefer to move some of the more involved enum rendering logic to crdb_internal.create_statements
Reviewed 2 of 6 files at r2, 8 of 8 files at r3.
Reviewable status:
pkg/cli/dump.go, line 187 at r1 (raw file):
Previously, knz (kena) wrote…
(I have commented elsewhere, including on #28948, that the current architecture is flawed - all this logic should be done server-side, in which case the cross-version compat story is much easier.)
this should be fine -- it is emitting a setting that is only known to 20.2+ it isn't sending it to the server. the types are only understood by a new server so it is fine if the setting is too.
pkg/cli/dump.go, line 362 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
heh, yeah I felt that I was taking it a step too far here. I'll extend the internal table I'm referencing to return the necessary information.
yeah, i'd also have preference for the crdb_internal.create_statements table doing more of this for us than baking a lot of logic into the client.
pkg/cmd/roachtest/dump.go, line 1 at r3 (raw file):
// Copyright 2019 The Cockroach Authors.
nit: 2020
There was a problem hiding this comment.
Reviewable status:
pkg/cli/dump.go, line 362 at r1 (raw file):
Previously, dt (David Taylor) wrote…
yeah, i'd also have preference for the crdb_internal.create_statements table doing more of this for us than baking a lot of logic into the client.
I'm not sure what else I can push to the server. Unfortunately, the driver doesn't return arrays as []string, but rather as []byte that needs to be decoded into an array.
rohany
force-pushed
the
udt-dump
branch
2 times, most recently
from
e8f8d77
to
1584411
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/cli/dump.go, line 362 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
I'm not sure what else I can push to the server. Unfortunately, the driver doesn't return arrays as []string, but rather as []byte that needs to be decoded into an array.
oh, sorry, i was looking at wrong revision in reviewable when i thought that -- this LGTM
Fixes cockroachdb#47765. This PR enables `cockroach dump` to dump databases and tables that contain user defined types. Release note (cli change): Allow for `cockroach dump` to dump tables and databases that contain user defined types.
|
TFTR! bors r=dt |
Build succeeded |
Fixes #47765.
This PR enables
cockroach dumpto dump databases and tables thatcontain user defined types.
Release note (cli change): Allow for
cockroach dumpto dump tables anddatabases that contain user defined types.