Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli/dump: more clearly inform the user upon tables with no visible columns #38725

Merged
merged 2 commits into from
Jul 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions pkg/cli/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand Down
28 changes: 26 additions & 2 deletions pkg/cli/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ func (c cliTest) runWithArgsUnredirected(origArgs []string) {
return Run(args)
}(); err != nil {
fmt.Println(err)
maybeShowErrorDetails(os.Stdout, err, false /*printNewLine*/)
}
}

Expand Down Expand Up @@ -1859,8 +1860,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
Expand Down Expand Up @@ -2552,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
}
16 changes: 15 additions & 1 deletion pkg/cli/dump.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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,
Expand Down