From 861dc403cdbf60efa53876a07c0a66547363e002 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 8 Jul 2019 14:38:10 +0200 Subject: [PATCH 1/2] cli/Main: show user details and hints included in error if available Release note: None --- pkg/cli/cli.go | 12 ++++++++++++ pkg/cli/cli_test.go | 7 +++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/pkg/cli/cli.go b/pkg/cli/cli.go index 44a706616010..9964a776a641 100644 --- a/pkg/cli/cli.go +++ b/pkg/cli/cli.go @@ -67,7 +67,15 @@ func Main() { errCode := 0 if err := Run(os.Args[1:]); err != nil { + // Display the error and its details/hints. + fmt.Fprintln(stderr, "Error:", err.Error()) + maybeShowErrorDetails(stderr, err, false /* printNewline */) + + // Remind the user of which command was being run. fmt.Fprintf(stderr, "Failed running %q\n", cmdName) + + // Finally, extract the error code, as optionally specified + // by the sub-command. errCode = 1 if ec, ok := errors.Cause(err).(*cliError); ok { errCode = ec.exitCode @@ -145,6 +153,10 @@ var cockroachCmd = &cobra.Command{ // Commands should manually print usage information when the error is, // in fact, a result of a bad invocation, e.g. too many arguments. SilenceUsage: true, + // Disable automatic printing of the error. We want to also print + // details and hints, which cobra does not do for us. Instead + // we do the printing in Main(). + SilenceErrors: true, } func init() { diff --git a/pkg/cli/cli_test.go b/pkg/cli/cli_test.go index 2f8b8fe7c03a..38517e753f30 100644 --- a/pkg/cli/cli_test.go +++ b/pkg/cli/cli_test.go @@ -1859,8 +1859,11 @@ Use "cockroach [command] --help" for more information about a command. done <- err }() - if err := Run(test.flags); err != nil && !test.expErr { - t.Error(err) + if err := Run(test.flags); err != nil { + fmt.Fprintln(w, "Error:", err) + if !test.expErr { + t.Error(err) + } } // back to normal state From d4b257bf06a4521d96f14bef068b3f8bad71a1b1 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 8 Jul 2019 14:38:41 +0200 Subject: [PATCH 2/2] cli/dump: inform the user clearly when dumping no-col tables Before: ``` kena@kenax ~/cockroach % ./cockroach dump --insecure defaultdb CREATE TABLE t (, FAMILY "primary" (rowid) ); Error: pq: at or near "from": syntax error Failed running "dump" ``` After: ``` kena@kenax ~/cockroach % ./cockroach dump --insecure defaultdb CREATE TABLE t (, FAMILY "primary" (rowid) ); Error: table "defaultdb.public.t" has no visible columns HINT: To proceed with the dump, either omit this table from the list of tables to dump, drop the table, or add some visible columns. -- See: https://github.com/cockroachdb/cockroach/issues/37768 Failed running "dump" ``` Release note (cli change): `cockroach dump` will now more clearly refer to issue #37768 when it encounters a table with no visible columns, which (currently) cannot be dumped successfully. --- pkg/cli/cli_test.go | 21 +++++++++++++++++++++ pkg/cli/dump.go | 16 +++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/pkg/cli/cli_test.go b/pkg/cli/cli_test.go index 38517e753f30..7d3a699a0fb0 100644 --- a/pkg/cli/cli_test.go +++ b/pkg/cli/cli_test.go @@ -382,6 +382,7 @@ func (c cliTest) runWithArgsUnredirected(origArgs []string) { return Run(args) }(); err != nil { fmt.Println(err) + maybeShowErrorDetails(os.Stdout, err, false /*printNewLine*/) } } @@ -2555,3 +2556,23 @@ func Example_sqlfmt() { // sqlfmt --no-simplify -e select (1+2)+3 // SELECT (1 + 2) + 3 } + +func Example_dump_no_visible_columns() { + c := newCLITest(cliTestParams{}) + defer c.cleanup() + + c.RunWithArgs([]string{"sql", "-e", "create table t(x int); set sql_safe_updates=false; alter table t drop x"}) + c.RunWithArgs([]string{"dump", "defaultdb"}) + + // Output: + // sql -e create table t(x int); set sql_safe_updates=false; alter table t drop x + // ALTER TABLE + // dump defaultdb + // CREATE TABLE t (, + // FAMILY "primary" (rowid) + // ); + // table "defaultdb.public.t" has no visible columns + // HINT: To proceed with the dump, either omit this table from the list of tables to dump, drop the table, or add some visible columns. + // -- + // See: https://github.com/cockroachdb/cockroach/issues/37768 +} diff --git a/pkg/cli/dump.go b/pkg/cli/dump.go index 0023e92f1b15..5394e4795d6b 100644 --- a/pkg/cli/dump.go +++ b/pkg/cli/dump.go @@ -29,7 +29,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/ctxgroup" "github.com/cockroachdb/cockroach/pkg/util/timeofday" "github.com/cockroachdb/cockroach/pkg/util/version" - "github.com/pkg/errors" + "github.com/cockroachdb/errors" "github.com/spf13/cobra" ) @@ -538,6 +538,20 @@ func dumpTableData(w io.Writer, conn *sqlConn, clusterTS string, bmd basicMetada return err } + if strings.TrimSpace(md.columnNames) == "" { + // A table with no columns may still have one or more rows. In + // fact, it can have arbitrary many rows, each with a different + // (hidden) PK value. Unfortunately, the dump command today simply + // omits the hidden PKs from the dump, so it is not possible to + // restore the invisible values. + // Instead of failing with an incomprehensible error below, inform + // the user more clearly. + err := errors.Newf("table %q has no visible columns", tree.ErrString(bmd.name)) + err = errors.WithHint(err, "To proceed with the dump, either omit this table from the list of tables to dump, drop the table, or add some visible columns.") + err = errors.WithIssueLink(err, errors.IssueLink{IssueURL: "https://github.com/cockroachdb/cockroach/issues/37768"}) + return err + } + bs := fmt.Sprintf("SELECT %s FROM %s AS OF SYSTEM TIME %s ORDER BY PRIMARY KEY %[2]s", md.columnNames, md.name,