sql: SHOW TABLES marks a dropped table with a " (dropped)" suffix #10063

vivekmenezes · 2016-10-18T20:10:38Z

Added another commit that renames table deleted to table dropped for #9493

This change is

dt · 2016-10-18T20:20:48Z

hm, how do we feel about show tables and the introspection meta tables (pg_* and information_schema) being different?

dt · 2016-10-18T20:21:36Z

I wonder if we should just change desc.Name in-place when we flip the Deleted bit, so all the paths read the same thing

vivekmenezes · 2016-10-18T20:28:02Z

I'm not familiar with what you're talking about. What's broken here?

dt · 2016-10-18T20:31:12Z

in addition to SHOW TABLES, we support reflecting on tables using the metatables in information_schema or pg_catalog. Having show tables show one name while they show something else is potentially confusing (plus there's a goal to unify their implementation, potentially replacing the SHOW commands with queries into the meta tables). While this isn't broken yet, it just introduces inconsistency when it's only implemented here.

knz · 2016-10-18T23:28:42Z

I'd recommend linking this with #9966.

andreimatei · 2016-10-19T02:09:07Z

LGTM

this looks to me like a quick fix for something that has come up many times. Whether or not the table should appear in information schema is a valid question; I'd say leave it alone there, since we probably shouldn't be massaging names like we're doing in the SHOW output.
Once we back the implementation of SHOW TABLES with queries in info schema, we can still continue doing this massaging, I think that's fair.

andreimatei · 2016-10-19T02:04:09Z

pkg/sql/lease_test.go

+
+	// Check that SHOW TABLES marks a deleted table with the " (deleted)"
+	// suffix.
+	rows, err := t.db.Query(`SHOW TABLES FROM test`)


defer rows.Close()

andreimatei · 2016-10-19T02:05:16Z

pkg/sql/lease_test.go

@@ -401,7 +401,8 @@ func TestLeaseManagerPublishVersionChanged(testingT *testing.T) {
 	t.expectLeases(descID, "/3/1")
 }

-// Test that we fail to lease a table that was marked for deletion.
+// Test that we fail to lease a table that was marked for deletion. This also
+// tests that SHOW TABLES marks a deleted table with the " (deleted)" suffix.


I think it'd be more natural for this to be a test on SHOW TABLES

Done. Moved to drop_test.go as way for us to test a variety of operations while a table is being dropped.

Change the error message when a DROP TABLE tried to drop a table that has already been dropped. Added tests for error messages observed with CREATE and DROP commands while a table is being dropped. fixes cockroachdb#9318

vivekmenezes · 2016-10-27T18:05:59Z

Any thoughts on this PR?

andreimatei · 2016-10-27T18:53:09Z

Still LGTM as far as I'm concerned.
Was there a particular point to r3? If not, there is a cost to churn...

Review status: 0 of 9 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

pkg/sql/drop_test.go, line 502 at r2 (raw file):

      t.Fatal("table invisible through SHOW TABLES")
  }
  var val []byte

can't this be a string?

Comments from Reviewable

vivekmenezes · 2016-10-27T19:13:57Z

The dropped versus deleted incompatibility was driving me crazy!

Review status: 0 of 9 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

pkg/sql/drop_test.go, line 502 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

can't this be a string?

Yes it can. I was under the impression that a string was immutable: https://golang.org/ref/spec#String_types

I'm puzzled by what they mean by immutable

Comments from Reviewable

andreimatei · 2016-10-27T19:45:24Z

Review status: 0 of 9 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

pkg/sql/drop_test.go, line 502 at r2 (raw file):

Previously, vivekmenezes wrote…

Yes it can. I was under the impression that a string was immutable: https://golang.org/ref/spec#String_types

I'm puzzled by what they mean by immutable

the mean that you can't change the contents of a string instance. Here you're passing a pointer to a string to `Scan`, and the instance being pointed to will be overwritten by a completely new instance.

Comments from Reviewable

tamird · 2016-10-27T21:12:00Z

Reviewed 9 of 9 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

pkg/sql/drop_test.go, line 470 at r3 (raw file):

  params.Knobs = base.TestingKnobs{
      SQLSchemaChanger: &sql.SchemaChangerTestingKnobs{
          SyncFilter: func(tscc sql.TestingSchemaChangerCollection) {

FYI: this can be written as SyncFilter: (*sql.TestingSchemaChangerCollection).ClearSchemaChangers. Just sharing.

pkg/sql/show.go, line 668 at r3 (raw file):

              tableName := name.Table()
              // Check to see if the table has been dropped.
              if _, err := p.mustGetTableOrViewDesc(&name); err != nil {

NYC but a function called mustGet... returns an error? What has the world come to?

Comments from Reviewable

…nation raft: fix correctness bug in CommittedEntries pagination

vivekmenezes assigned andreimatei Oct 18, 2016

andreimatei reviewed Oct 19, 2016

View reviewed changes

vivekmenezes force-pushed the showtables branch 4 times, most recently from 131deae to 973b2f8 Compare October 21, 2016 19:50

vivekmenezes changed the title ~~sql: SHOW TABLES marks a deleted table with a " (deleted)" suffix~~ sql: SHOW TABLES marks a dropped table with a " (dropped)" suffix Oct 21, 2016

vivekmenezes force-pushed the showtables branch 2 times, most recently from 0094377 to 8651da9 Compare October 21, 2016 21:37

vivekmenezes mentioned this pull request Oct 25, 2016

sql: schema change after server restart blocks once then looks like DB corruption after retry #9493

Closed

sql: SHOW TABLES marks a dropped table with a (dropped) suffix

8ba8b97

Change the error message when a DROP TABLE tried to drop a table that has already been dropped. Added tests for error messages observed with CREATE and DROP commands while a table is being dropped. fixes cockroachdb#9318

vivekmenezes force-pushed the showtables branch from 8651da9 to 781687b Compare October 27, 2016 17:35

sql: rename table deleted to table dropped

cbb636d

vivekmenezes force-pushed the showtables branch from 781687b to cbb636d Compare October 27, 2016 19:10

vivekmenezes merged commit e96b448 into cockroachdb:master Oct 27, 2016

vivekmenezes deleted the showtables branch October 27, 2016 19:57

jseldess added the docs-todo label Oct 31, 2016

jseldess mentioned this pull request Oct 31, 2016

sql: SHOW TABLES marks a dropped table with a " (dropped)" suffix cockroachdb/docs#808

Closed

jseldess added the docs-done label Nov 3, 2016

jseldess removed the docs-todo label Nov 3, 2016

pav-kv pushed a commit to pav-kv/cockroach that referenced this pull request Mar 5, 2024

Merge pull request cockroachdb#10063 from tschottdorf/fix-commit-pagi…

63d2b7e

…nation raft: fix correctness bug in CommittedEntries pagination

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: SHOW TABLES marks a dropped table with a " (dropped)" suffix #10063

sql: SHOW TABLES marks a dropped table with a " (dropped)" suffix #10063

vivekmenezes commented Oct 18, 2016 •

edited

Loading

dt commented Oct 18, 2016

dt commented Oct 18, 2016

vivekmenezes commented Oct 18, 2016

dt commented Oct 18, 2016

knz commented Oct 18, 2016

andreimatei commented Oct 19, 2016

andreimatei Oct 19, 2016

vivekmenezes Oct 21, 2016

andreimatei Oct 19, 2016

vivekmenezes Oct 21, 2016 •

edited

Loading

vivekmenezes commented Oct 27, 2016

andreimatei commented Oct 27, 2016

vivekmenezes commented Oct 27, 2016

andreimatei commented Oct 27, 2016

tamird commented Oct 27, 2016

sql: SHOW TABLES marks a dropped table with a " (dropped)" suffix #10063

sql: SHOW TABLES marks a dropped table with a " (dropped)" suffix #10063

Conversation

vivekmenezes commented Oct 18, 2016 • edited Loading

dt commented Oct 18, 2016

dt commented Oct 18, 2016

vivekmenezes commented Oct 18, 2016

dt commented Oct 18, 2016

knz commented Oct 18, 2016

andreimatei commented Oct 19, 2016

andreimatei Oct 19, 2016

Choose a reason for hiding this comment

vivekmenezes Oct 21, 2016

Choose a reason for hiding this comment

andreimatei Oct 19, 2016

Choose a reason for hiding this comment

vivekmenezes Oct 21, 2016 • edited Loading

Choose a reason for hiding this comment

vivekmenezes commented Oct 27, 2016

andreimatei commented Oct 27, 2016

vivekmenezes commented Oct 27, 2016

andreimatei commented Oct 27, 2016

tamird commented Oct 27, 2016

vivekmenezes commented Oct 18, 2016 •

edited

Loading

vivekmenezes Oct 21, 2016 •

edited

Loading