Skip to content

Commit

Permalink
sql: allow the InternalExecutor to work with a modified schema
Browse files Browse the repository at this point in the history
The internal executor uses its own TableCollection. This is
fine normally but is a problem when it is called in the context
of a transaction that has already modified schema. The parent
transaction will leave an outstanding intent on the schema,
and the InternalExecutor will block attempting to acquire a
lease on the schema. The fix is to modify the TableCollection
used by the InternalExecutor to house the modified schema
so that a schema lookup made by the InternalExecutor will not
attempt to acquire a lease.

This change is only creating the infrastructure by which this
is done but not fixing the underlying deadlock problem.

related to cockroachdb#34304

Release note: None
  • Loading branch information
vivekmenezes committed Feb 6, 2019
1 parent 844d751 commit 1e05a9b
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 1 deletion.
8 changes: 8 additions & 0 deletions pkg/sql/conn_executor.go
Expand Up @@ -607,6 +607,7 @@ func (s *Server) newConnExecutorWithTxn(
memMetrics MemoryMetrics,
srvMetrics *Metrics,
txn *client.Txn,
tcModifier TableCollectionModifier,
) (*connExecutor, error) {
ex, err := s.newConnExecutor(ctx, sargs, stmtBuf, clientComm, memMetrics, srvMetrics)
if err != nil {
Expand All @@ -633,6 +634,13 @@ func (s *Server) newConnExecutorWithTxn(
tree.ReadWrite,
txn,
ex.transitionCtx)

// Modify the TableCollection to match the parent executor's TableCollection.
// This allows the InternalExecutor to see schema changes made by the
// parent executor.
if tcModifier != nil {
tcModifier.copyModifiedSchema(&ex.extraTxnState.tables)
}
return ex, nil
}

Expand Down
12 changes: 11 additions & 1 deletion pkg/sql/internal.go
Expand Up @@ -83,6 +83,15 @@ type internalExecutorImpl struct {
// parent session state if need be, but then we'd need to share a
// sessionDataMutator.
sessionData *sessiondata.SessionData

// The internal executor uses its own TableCollection. A TableCollection
// is a schema cache for each transaction and contains data like the schema
// modified by a transaction. Occasionally an internal executor is called
// within the context of a transaction that has modified the schema, the
// internal executor should see the modified schema. This interface allows
// the internal executor to modify its TableCollection to match the
// TableCollection of the parent executor.
tcModifier TableCollectionModifier
}

// MakeInternalExecutor creates an InternalExecutor.
Expand Down Expand Up @@ -192,7 +201,8 @@ func (ie *internalExecutorImpl) initConnEx(
ie.mon,
ie.memMetrics,
&ie.s.InternalMetrics,
txn)
txn,
ie.tcModifier)
}
if err != nil {
return nil, nil, err
Expand Down
17 changes: 17 additions & 0 deletions pkg/sql/table.go
Expand Up @@ -658,6 +658,23 @@ func (tc *TableCollection) releaseAllDescriptors() {
tc.allDescriptors = nil
}

// Copy the modified schema to the table collection. Used when initializing
// an InternalExecutor.
func (tc *TableCollection) copyModifiedSchema(to *TableCollection) {
if tc == nil {
return
}
to.uncommittedTables = tc.uncommittedTables
to.uncommittedDatabases = tc.uncommittedDatabases
// Do not copy the leased descriptors because we do not want
// the leased descriptors to be released by the "to" TableCollection.
// The "to" TableCollection can re-lease the same descriptors.
}

type TableCollectionModifier interface {
copyModifiedSchema(to *TableCollection)
}

// createOrUpdateSchemaChangeJob finalizes the current mutations in the table
// descriptor. If a schema change job in the system.jobs table has not been
// created for mutations in the current transaction, one is created. The
Expand Down

0 comments on commit 1e05a9b

Please sign in to comment.