cli: add import pgdump/mysqldump CLI command by adityamaru · Pull Request #54896 · cockroachdb/cockroach

adityamaru · 2020-09-29T00:00:34Z

This change introduces a new CLI command to import
locally saved PGDUMP or MYSQLDUMP files into a running
cockroach cluster.

The underlying logic relies on user scoped userfile
storage to upload the local dump file to, and subsequently
import data from. Most of the heavy lifting was already
completed during the development of the userfile storage.

We can only support bundle formats that have the table
schemas to be imported, baked into the dump files. The newly
added CLI commands support the same options as IMPORT PGDUMP
and IMPORT MYSQLDUMP from the SQL shell do. These options
are to be passed in as CLI flags.

Following are the CLI examples:
./cockroach import db <format> <source>
./cockroach import table <tablename> <format> <source>

Release note (cli change): Adds an import CLI command which
allows users to upload and import local dump files into a
running cockroach cluster. We currently support PGDUMP and
MYSQLDUMP formats.

cockroach-teamcity · 2020-09-29T00:00:42Z

This change is

adityamaru · 2020-09-29T00:04:02Z

@knz I marked this as a WIP cause I'm running into a weird issue when trying to write tests for this CLI command. The conn.ensureConn method in runImport (https://github.com/cockroachdb/cockroach/pull/54896/files#diff-c5484a64be947c73503bb9e7ccbe71e1R71) is returning an EOF error when run through unit tests. When running against a local cockroach node all the commands work as expected. The EOF is apparently being throw because of a failed TCP handshake in the ensureConn method.

Spent a bunch of time trying to see what was different between this command and other existing ones but have not been able to spot anything yet, wondering if you see something amiss! I'll keep digging tomorrow.

knz

I'm running into a weird issue when trying to write tests for this CLI command.

I think you need to update isSQLCommand in cli_test.go.

The rest LGTM as far as the CLI code goes. I'll let your team comment on security and other UX aspects.

Reviewed 10 of 10 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @miretskiy)

pkg/cli/userfile.go, line 269 at r1 (raw file):

	}

	files, err := f.ListFiles(ctx, "")

return f.ListFiles(...)

pkg/cli/userfile.go, line 391 at r1 (raw file):

}

func uploadUserFile(

please add a comment to explain what the function does and the meaning of its return values.

knz · 2020-09-30T09:49:59Z

I'm running into a weird issue when trying to write tests for this CLI command.

If you rebase it should be fixed now. (#54932)

adityamaru · 2020-09-30T15:58:18Z

I'm running into a weird issue when trying to write tests for this CLI command.

If you rebase it should be fixed now. (#54932)

Thank you @knz!!

adityamaru · 2020-10-14T13:10:41Z

@miretskiy this is RFAL now, thanks!

miretskiy

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @knz)

pkg/cli/import.go, line 34 at r2 (raw file):

var importDumpTableCmd = &cobra.Command{
	Use:   "table <table> <format> <source>",

do we want to support multiple tables?

pkg/cli/import.go, line 82 at r2 (raw file):

	defer conn.Close()
	ctx := context.Background()
	return runImport(ctx, conn, importFormat, source, tableName, true /* isTableImport */)

maybe add a typedef instead?

knz · 2020-10-22T10:31:27Z

btw how does this work with user-defined schemas?

adityamaru

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @miretskiy)

pkg/cli/import.go, line 34 at r2 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

do we want to support multiple tables?

importDumpFileCmd above is the flavour that supports the entire database. Or are you suggesting something in between a single table and the whole db?

pkg/cli/import.go, line 82 at r2 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

maybe add a typedef instead?

Done.

pkg/cli/userfile.go, line 269 at r1 (raw file):

Previously, knz (kena) wrote…

return f.ListFiles(...)

Done.

pkg/cli/userfile.go, line 391 at r1 (raw file):

Previously, knz (kena) wrote…

please add a comment to explain what the function does and the meaning of its return values.

Done.

adityamaru · 2020-10-22T13:40:09Z

btw how does this work with user-defined schemas?

https://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/importccl/import_stmt.go#L784
We don't allow IMPORT to target user-defined schemas.

knz · 2020-10-22T13:51:58Z

We don't allow IMPORT to target user-defined schemas.

Yes I understand, but can I import a table into a different schema than public?

Also what happens if I try to import a table into pg_temp? Does this work or do I get surprises?

adityamaru · 2020-10-22T14:48:24Z

We don't allow IMPORT to target user-defined schemas.

Yes I understand, but can I import a table into a different schema than public?

Also what happens if I try to import a table into pg_temp? Does this work or do I get surprises?

I may be misunderstanding but the line I linked above only allows import to work on public schemas. So in my testing, supplying either a user-defined schema or a virtual schema throws an error cannot import into a user-defined schema x as expected. The commands I tested with are:
./cockroach import table defaultdb.foo.simple pgdump pkg/ccl/importccl/testdata/pgdump/simple.sql --insecure
./cockroach import table defaultdb.pg_catalog.simple pgdump pkg/ccl/importccl/testdata/pgdump/simple.sql --insecure

re: pg_temp, the way I understand it is that the import CLI command will create a new SQLConnection, and since the lifetime of pg_temp is tied to a session, our import CLI command can never see a pg_temp schema from another SQL connection. It currently returns an error database does not exist: "defaultdb.pg_temp.simple" which is as expected in my opinion.

miretskiy

Reviewed 1 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @knz, and @miretskiy)

pkg/cli/import.go, line 34 at r2 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

importDumpFileCmd above is the flavour that supports the entire database. Or are you suggesting something in between a single table and the whole db?

Right... Like import these 3 tables from the dump file.

knz

Reviewed 4 of 6 files at r2, 1 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @miretskiy)

pkg/cli/import_test.go, line 31 at r3 (raw file):

) string {
	knobs, unsetImportCLIKnobs := setImportCLITestingKnobs()
	defer unsetImportCLIKnobs()

Some explanatory comments around the pieces at the start of this function would help comprehension, thanks.

This change introduces a new CLI command to import locally saved PGDUMP or MYSQLDUMP files into a running cockroach cluster. The underlying logic relies on user scoped userfile storage to upload the local dump file to, and subsequently import data from. Most of the heavy lifting was already completed during the development of the userfile storage. We can only support bundle formats which have the table schemas to be imported, baked into the dump files. The newly added CLI commands support the same options as IMPORT PGDUMP and IMPORT MYSQLDUMP from the SQL shell do. These options are to be passed in as CLI flags. Following are the CLI examples: `./cockroach import db <format> <source>` `./cockroach import table <tablename> <format> <source>` Release note (cli change): Adds an import CLI command which allows users to upload and import local dump files into a running cockroach cluster. We currently support PGDUMP and MYSQLDUMP formats.

adityamaru

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @miretskiy)

pkg/cli/import.go, line 34 at r2 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

Right... Like import these 3 tables from the dump file.

I'll add it in a follow-up PR after bringing it up with Michael this week to get his product perspective. The change should be minimal.

pkg/cli/import_test.go, line 31 at r3 (raw file):

Previously, knz (kena) wrote…

Some explanatory comments around the pieces at the start of this function would help comprehension, thanks.

Done.

adityamaru · 2020-10-26T15:34:21Z

TFTRs!

bors r=miretskiy,knz

54896: cli: add import pgdump/mysqldump CLI command r=miretskiy,knz a=adityamaru This change introduces a new CLI command to import locally saved PGDUMP or MYSQLDUMP files into a running cockroach cluster. The underlying logic relies on user scoped userfile storage to upload the local dump file to, and subsequently import data from. Most of the heavy lifting was already completed during the development of the userfile storage. We can only support bundle formats that have the table schemas to be imported, baked into the dump files. The newly added CLI commands support the same options as IMPORT PGDUMP and IMPORT MYSQLDUMP from the SQL shell do. These options are to be passed in as CLI flags. Following are the CLI examples: `./cockroach import db <format> <source>` `./cockroach import table <tablename> <format> <source>` Release note (cli change): Adds an import CLI command which allows users to upload and import local dump files into a running cockroach cluster. We currently support PGDUMP and MYSQLDUMP formats. 55687: *: bazel-ify cockroachdb r=irfansharif a=irfansharif This commit introduces a new build system to cockroachdb: Bazel. The hope here is to eventually replace our current Makefile. It sets the foundation for introducing Bazel in our CI systems for parallelized builds/test runs, artifact caching, remote builds, and many other such profits (literally, it could reduce infra spend). We originally prototyped a pseudo bazel-ified cockroachdb in #52824, which resulted in a week long effort (with otan@, jamesl@, and myself) to wrap it up in earnest in #55258. This is the polished version of the resulting work, the write up for which can be found at http://go.crdb.dev/bazel-hackweek. --- Most of the BUILD files here were auto-generated through Gazelle (see doc). Some of these auto-generated files required further manual tweaking to get just right (see doc). These were mostly around the packages that link into c-dependencies, but also the few packages that rely on auto-generated code (again, see doc). These specific files are the ones intended for review (they're also the ones responsible for auto-generation), among a few others. I've listed them out here: ``` new file: .bazelrc modified: .gitattributes modified: .gitignore new file: BUILD.bazel new file: DEPS.bzl modified: Makefile new file: WORKSPACE new file: pkg/ccl/gssapiccl/BUILD.bazel new file: pkg/ccl/storageccl/engineccl/BUILD.bazel new file: pkg/cli/BUILD.bazel new file: pkg/cmd/cockroach-oss/BUILD.bazel new file: pkg/cmd/cockroach-short/BUILD.bazel new file: pkg/cmd/cockroach/BUILD.bazel new file: pkg/col/coldata/BUILD.bazel new file: pkg/geo/geoproj/BUILD.bazel modified: pkg/geo/geoproj/geoproj.go modified: pkg/geo/geoproj/proj.cc new file: pkg/geo/geos/BUILD.bazel new file: pkg/sql/colconv/BUILD.bazel new file: pkg/sql/colexec/BUILD.bazel new file: pkg/sql/colexec/COLEXEC.bzl modified: pkg/sql/colexec/and_or_projection_tmpl.go modified: pkg/sql/colexec/any_not_null_agg_tmpl.go modified: pkg/sql/colexec/avg_agg_tmpl.go modified: pkg/sql/colexec/bool_and_or_agg_tmpl.go modified: pkg/sql/colexec/cast_tmpl.go new file: pkg/sql/colexec/colbuilder/BUILD.bazel new file: pkg/sql/colexec/colexec.go modified: pkg/sql/colexec/concat_agg_tmpl.go modified: pkg/sql/colexec/const_tmpl.go modified: pkg/sql/colexec/count_agg_tmpl.go modified: pkg/sql/colexec/default_agg_tmpl.go modified: pkg/sql/colexec/default_cmp_expr_tmpl.go modified: pkg/sql/colexec/default_cmp_proj_ops_tmpl.go modified: pkg/sql/colexec/default_cmp_sel_ops_tmpl.go modified: pkg/sql/colexec/distinct_tmpl.go modified: pkg/sql/colexec/hash_aggregator_tmpl.go modified: pkg/sql/colexec/hash_utils_tmpl.go modified: pkg/sql/colexec/hashjoiner_tmpl.go modified: pkg/sql/colexec/hashtable_tmpl.go modified: pkg/sql/colexec/is_null_ops_tmpl.go modified: pkg/sql/colexec/mergejoinbase_tmpl.go modified: pkg/sql/colexec/mergejoiner_tmpl.go modified: pkg/sql/colexec/min_max_agg_tmpl.go modified: pkg/sql/colexec/ordered_synchronizer_tmpl.go modified: pkg/sql/colexec/proj_const_ops_tmpl.go modified: pkg/sql/colexec/proj_non_const_ops_tmpl.go modified: pkg/sql/colexec/quicksort_tmpl.go modified: pkg/sql/colexec/rank_tmpl.go modified: pkg/sql/colexec/relative_rank_tmpl.go modified: pkg/sql/colexec/row_number_tmpl.go modified: pkg/sql/colexec/rowstovec_tmpl.go modified: pkg/sql/colexec/select_in_tmpl.go modified: pkg/sql/colexec/selection_ops_tmpl.go modified: pkg/sql/colexec/sort_tmpl.go modified: pkg/sql/colexec/substring_tmpl.go modified: pkg/sql/colexec/sum_agg_tmpl.go modified: pkg/sql/colexec/utils_tmpl.go modified: pkg/sql/colexec/values_differ_tmpl.go modified: pkg/sql/colexec/vec_comparators_tmpl.go modified: pkg/sql/colexec/window_peer_grouper_tmpl.go new file: pkg/sql/colflow/BUILD.bazel new file: pkg/sql/opt/BUILD.bazel new file: pkg/sql/opt/exec/BUILD.bazel new file: pkg/sql/opt/exec/execbuilder/BUILD.bazel new file: pkg/sql/opt/exec/explain/BUILD.bazel new file: pkg/sql/opt/memo/BUILD.bazel new file: pkg/sql/opt/norm/BUILD.bazel new file: pkg/sql/opt/optgen/lang/BUILD.bazel new file: pkg/sql/opt/xform/BUILD.bazel new file: pkg/sql/parser/BUILD.bazel new file: pkg/sql/parser/sql-gen.sh modified: pkg/sql/parser/sql.y new file: pkg/storage/BUILD.bazel modified: vendor ``` The changes to all the _tmpl.go files were needed to include necessary import analysis/compilation for bazel when generating the .eg.go files (also summarized in doc up above), and should be uncontroversial. I think it's easiest to pull the branch down locally and to click around to see what works, and what doesn't. I would start at the top-level BUILD.bazel and WORSPACE file. I've added comments to explain what most of added machinery does. Release note: None 55827: sql: stub ALTER TABLE ... SET LOCALITY/AFFINITY related commands r=ajstorm a=otan Following the doc on proposed ideas and fleshed them all out. They default to displaying REGIONAL AFFINITY for now. Release note (sql change): Implemented ALTER TABLE ... SET LOCALITY/REGIONAL AFFINITY commands that allow users to configure multiregion properties of given tables. These may change later. Co-authored-by: Aditya Maru <adityamaru@gmail.com> Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com> Co-authored-by: Oliver Tan <otan@cockroachlabs.com>

craig · 2020-10-26T16:00:33Z

This PR was included in a batch that was canceled, it will be automatically retried

craig · 2020-10-26T17:02:14Z

Build succeeded:

GitHub CI (Cockroach)

adityamaru requested review from knz and miretskiy September 29, 2020 00:00

adityamaru requested a review from a team as a code owner September 29, 2020 00:00

knz reviewed Sep 29, 2020

View reviewed changes

adityamaru force-pushed the import-cli branch from 996002b to e7c90c5 Compare October 14, 2020 13:05

adityamaru changed the title ~~[WIP] cli: add import pgdump/mysqldump CLI command~~ cli: add import pgdump/mysqldump CLI command Oct 14, 2020

miretskiy requested a review from knz October 21, 2020 16:45

miretskiy suggested changes Oct 21, 2020

View reviewed changes

adityamaru force-pushed the import-cli branch from e7c90c5 to 1a737ec Compare October 22, 2020 13:36

adityamaru commented Oct 22, 2020

View reviewed changes

adityamaru requested a review from miretskiy October 22, 2020 13:46

miretskiy approved these changes Oct 22, 2020

View reviewed changes

miretskiy self-requested a review October 22, 2020 17:45

knz approved these changes Oct 23, 2020

View reviewed changes

adityamaru force-pushed the import-cli branch from 1a737ec to 1a2a301 Compare October 26, 2020 14:01

adityamaru commented Oct 26, 2020

View reviewed changes

craig bot merged commit b0b53c7 into cockroachdb:master Oct 26, 2020

jseldess mentioned this pull request Dec 9, 2020

cli: add import pgdump/mysqldump CLI command cockroachdb/docs#9139

Closed

Conversation

adityamaru commented Sep 29, 2020

Uh oh!

cockroach-teamcity commented Sep 29, 2020

Uh oh!

adityamaru commented Sep 29, 2020

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

knz commented Sep 30, 2020

Uh oh!

adityamaru commented Sep 30, 2020

Uh oh!

adityamaru commented Oct 14, 2020

Uh oh!

miretskiy left a comment

Choose a reason for hiding this comment

Uh oh!

knz commented Oct 22, 2020

Uh oh!

adityamaru left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Oct 22, 2020

Uh oh!

knz commented Oct 22, 2020

Uh oh!

adityamaru commented Oct 22, 2020

Uh oh!

miretskiy left a comment

Choose a reason for hiding this comment

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Oct 26, 2020

Uh oh!

craig bot commented Oct 26, 2020

Uh oh!

craig bot commented Oct 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants