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.

Sign up for GitHub

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

Jump to bottom

sql: fix lease interaction with offline tables #40996

Merged
merged 1 commit into from
Sep 23, 2019
Merged

sql: fix lease interaction with offline tables #40996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion pkg/ccl/importccl/import_stmt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1597,7 +1597,7 @@ func TestImportIntoCSV(t *testing.T) {
<-importBodyFinished

err := sqlDB.DB.QueryRowContext(ctx, `SELECT 1 FROM t`).Scan(&unused)
if !testutils.IsError(err, "table \"t\" is offline: importing") {
if !testutils.IsError(err, "relation \"t\" does not exist") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a blocker to a correctness fix, but I did kinda like the more descriptive user-visible error message (and we'd gotten feedback that liked the importing message from others as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the summary here is that the error we were catching was the one thrown when failing to get a lease on the table. this isn't exactly what we want - we want to see the table in the offline state and return it.

the actual PR that changed the error that we're returning here is #40285. That PR changed the logic from returning an error if the table was in a DROP of OFFLINE state to just reporting the object it was looking for as not there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking in more detail, it looks like that PR (40285) doesn't actually affect this as the behaviour described (returning no object found instead of the error) was always what was happening.

return err
}
return nil
Expand Down
9 changes: 5 additions & 4 deletions pkg/sql/lease.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ func (s LeaseStore) jitteredLeaseDuration() time.Duration {

// acquire a lease on the most recent version of a table descriptor.
// If the lease cannot be obtained because the descriptor is in the process of
// being dropped, the error will be errTableDropped.
// being dropped or offline, the error will be of type inactiveTableError.
// The expiration time set for the lease > minExpiration.
func (s LeaseStore) acquire(
ctx context.Context, minExpiration hlc.Timestamp, tableID sqlbase.ID,
Expand Down Expand Up @@ -980,7 +980,7 @@ func (t *tableState) removeInactiveVersions() []*storedTableLease {
}

// If the lease cannot be obtained because the descriptor is in the process of
// being dropped, the error will be errTableDropped.
// being dropped or offline, the error will be of type inactiveTableError.
// The boolean returned is true if this call was actually responsible for the
// lease acquisition.
func acquireNodeLease(ctx context.Context, m *LeaseManager, id sqlbase.ID) (bool, error) {
Expand Down Expand Up @@ -1138,8 +1138,9 @@ func purgeOldVersions(
// active lease, so that it doesn't get released when removeInactives()
// is called below. Release this lease after calling removeInactives().
table, _, err := t.findForTimestamp(ctx, m.clock.Now())
if dropped := err == errTableDropped; dropped || err == nil {
removeInactives(dropped)
if _, ok := err.(*inactiveTableError); ok || err == nil {
isInactive := ok
removeInactives(isInactive)
if table != nil {
s, err := t.release(&table.ImmutableTableDescriptor, m.removeOnceDereferenced())
if err != nil {
Expand Down
18 changes: 11 additions & 7 deletions pkg/sql/table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,20 +49,24 @@ func (p *planner) getVirtualTabler() VirtualTabler {
return p.extendedEvalCtx.VirtualSchemas
}

var errTableDropped = errors.New("table is being dropped")
var errTableAdding = errors.New("table is being added")

type inactiveTableError struct {
error
}

func filterTableState(tableDesc *sqlbase.TableDescriptor) error {
switch tableDesc.State {
case sqlbase.TableDescriptor_DROP:
return errTableDropped
case sqlbase.TableDescriptor_ADD:
return errTableAdding
return inactiveTableError{errors.New("table is being dropped")}
case sqlbase.TableDescriptor_OFFLINE:
err := errors.Errorf("table %q is offline", tableDesc.Name)
if tableDesc.OfflineReason != "" {
return errors.Errorf("table %q is offline: %s", tableDesc.Name, tableDesc.OfflineReason)
err = errors.Errorf("table %q is offline: %s", tableDesc.Name, tableDesc.OfflineReason)
}
return errors.Errorf("table %q is offline", tableDesc.Name)
return inactiveTableError{err}
case sqlbase.TableDescriptor_ADD:
return errTableAdding
case sqlbase.TableDescriptor_PUBLIC:
return nil
default:
Expand Down Expand Up @@ -284,7 +288,7 @@ func (tc *TableCollection) getTableVersion(
// Read the descriptor from the store in the face of some specific errors
// because of a known limitation of AcquireByName. See the known
// limitations of AcquireByName for details.
if err == errTableDropped || err == sqlbase.ErrDescriptorNotFound {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch!

if _, ok := err.(inactiveTableError); ok || err == sqlbase.ErrDescriptorNotFound {
return readTableFromStore()
}
// Lease acquisition failed with some other error. This we don't
Expand Down