sql: release table name as soon as it is no longer referenced

[vivekmenezes]

When dropping a table, the table name would be released only after
the table was truncated. Table truncation can take a long time and
it doesn't make for a good user experience when a user expects a
name for a dropped table to be available almost immediately.

fixes #7348

---

This change is 

[tamird]

Reviewed 3 of 3 files at r1.
Review status: 2 of 3 files reviewed at latest revision, 6 unresolved discussions, some commit checks pending.

---

pkg/sql/drop.go, line 980 at r1 (raw file):

	if err := db.Txn(ctx, func(txn *client.Txn) error {
		b := &client.Batch{}
		// Use CPut because we want this function to be idempotent.

that's not what idempotent means; this function will fail on retry. this is to prevent trampling unexpected values. I'd just remove the comment.

---

pkg/sql/drop.go, line 983 at r1 (raw file):

		b.CPut(nameKey, nil, tableDesc.ID)
		txn.SetSystemConfigTrigger()
		if err := txn.Run(b); err != nil {

err := txn.Run(b)
if _, ok := err.(*roachpb.ConditionFailedError); ok {
  return nil
}
return err

---

pkg/sql/drop.go, line 993 at r1 (raw file):

	}

	if testingKnobs.RunAfterTableNameDropped != nil {

nit:

if cb := testingKnobs.RunAfterTableNameDropped; cb != nil {
  if err := cb(); err != nil {
    return err
  }
}

---

pkg/sql/drop_test.go, line 324 at r1 (raw file):

				runAfterTableNameDropped = nil
				if runFunc != nil {
					go runFunc()

as suggested below, consider moving this go to the innermost part requiring asynchronous execution.

---

pkg/sql/drop_test.go, line 339 at r1 (raw file):

	numRows := 2*sql.TableTruncateChunkSize + 1
	createKVTable(t, sqlDB, numRows)
	var createWG sync.WaitGroup

this is an odd use for a waitgroup. how about an unbuffered error channel, instead? that also has the benefit of moving error handling to the main goroutine.

something like

runAfterTableNameDropped = func() {
_, err := sqlDB.Exec(`SELECT * FROM t.kv`)
go func() { errChan <- err }()
}

so as not to hold up the callback itself.

---

pkg/sql/drop_test.go, line 388 at r1 (raw file):

	checkKeyCount(t, kvDB, tablePrefix, 3*numRows)
	var wg sync.WaitGroup

this is an odd use for a waitgroup. how about an unbuffered error channel, instead? that also has the benefit of moving error handling to the main goroutine.

---

Comments from Reviewable

[vivekmenezes]

pkg/sql/drop.go, line 980 at r1 (raw file):

err := txn.Run(b)
if _, ok := err.(*roachpb.ConditionFailedError); ok {
return nil
}
return err

changed to

// Use CPut because we want to remove a specific name -> id map.

---

Comments from Reviewable

[vivekmenezes]

Review status: 2 of 3 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

pkg/sql/drop.go, line 983 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

err := txn.Run(b)
if _, ok := err.(*roachpb.ConditionFailedError); ok {
  return nil
}
return err

Done.

---

pkg/sql/drop.go, line 993 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit:

if cb := testingKnobs.RunAfterTableNameDropped; cb != nil {
  if err := cb(); err != nil {
    return err
  }
}

Done.

---

pkg/sql/drop_test.go, line 324 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

as suggested below, consider moving this go to the innermost part requiring asynchronous execution.

Not sure what this suggestion is. I've changed the code to use a chan error.

---

pkg/sql/drop_test.go, line 339 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is an odd use for a waitgroup. how about an unbuffered error channel, instead? that also has the benefit of moving error handling to the main goroutine.

something like

runAfterTableNameDropped = func() {
_, err := sqlDB.Exec(`SELECT * FROM t.kv`)
go func() { errChan <- err }()
}

so as not to hold up the callback itself.

Done.

---

pkg/sql/drop_test.go, line 388 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is an odd use for a waitgroup. how about an unbuffered error channel, instead? that also has the benefit of moving error handling to the main goroutine.

Done.

---

Comments from Reviewable

[jordanlewis]

Does this also resolve #12123?

[vivekmenezes]

It doesn't. We cannot resolve #12123 without some tradeoffs because the descriptors are cached across the cluster. In other words there is no transactional drop/create of a table, but we can change the behavior to be more user friendly.

[andreimatei]

LGTM
Good idea with this change!

---

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

---

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

	}

	// Drop the newly created table.

maybe say in this comment that the hook recreated the table. Or even better, move this drop into the hook.

---

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

	}

	// Test that deleted table cannot be used. This prevents regressions where

I think you can remove this assertion since you've copied it in the knob, right?

---

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

	}

	if gr, err := kvDB.Get(ctx, descKey); err != nil {

can this and the next checks be moved into the hook, before the table is recreated? If so, I think that'd be better since the test would be a little tighter.

---

Comments from Reviewable

[vivekmenezes]

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

---

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

Previously, andreimatei (Andrei Matei) wrote…

maybe say in this comment that the hook recreated the table. Or even better, move this drop into the hook.

Moved the DROP completely. See comment below.

---

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

Previously, andreimatei (Andrei Matei) wrote…

I think you can remove this assertion since you've copied it in the knob, right?

True, and so now I do not need to even DROP the table any more. Removed Drop from the hook. The hook is only used to check that the name can be used.

---

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

Previously, andreimatei (Andrei Matei) wrote…

can this and the next checks be moved into the hook, before the table is recreated? If so, I think that'd be better since the test would be a little tighter.

Only the nameKey can be moved there and I've done that.

---

Comments from Reviewable

[vivekmenezes]

I've also added a counter to check that the DROP table retry actually happens

---

Review status: 1 of 3 files reviewed at latest revision, 9 unresolved discussions.

---

Comments from Reviewable

[tamird]

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

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

			`SELECT * FROM t.kv`,
		); !testutils.IsError(err, `table "t.kv" does not exist`) {
			return errors.Wrap(err, "different error than expected")

this is incorrect. errors.Wrap(nil, ...) is nil.

---

Comments from Reviewable

[vivekmenezes]

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

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

Previously, tamird (Tamir Duberstein) wrote…

this is incorrect. errors.Wrap(nil, ...) is nil.

good catch!

#13971

---

Comments from Reviewable