Skip to content

Commit

Permalink
opt: fix race caused by shared table annotations
Browse files Browse the repository at this point in the history
When we assign placeholders on a detached memo, we copy the metadata.
However, this does a shallow copy of table annotations; this is
problematic for the statistics annotation which is a mutable object.

The fix is to register a copy function for each type of table
annotation as part of `NewTableAnnID`. For statistics, we simply clear
out the annotation because it can be recalculated as needed (and it
turns out to be faster than copying it).

Fixes cockroachdb#34904.

Release note (bug fix): Fixed a crash related to cached plans.
  • Loading branch information
RaduBerinde committed Feb 20, 2019
1 parent cc04b06 commit c1401e0
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 1 deletion.
16 changes: 15 additions & 1 deletion pkg/sql/opt/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ type Metadata struct {
// might resolve to the same object now but to different objects later; we
// want to verify the resolution of both names.
deps []mdDep

// NOTE! When adding fields here, update CopyFrom.
}

type mdDep struct {
Expand Down Expand Up @@ -132,13 +134,25 @@ func (md *Metadata) Init() {

// CopyFrom initializes the metadata with a copy of the provided metadata.
// This metadata can then be modified independent of the copied metadata.
//
// Table annotations are not transferred over; all annotations are unset on
// the copy.
func (md *Metadata) CopyFrom(from *Metadata) {
if len(md.cols) != 0 || len(md.tables) != 0 || len(md.deps) != 0 {
if len(md.schemas) != 0 || len(md.cols) != 0 || len(md.tables) != 0 ||
len(md.sequences) != 0 || len(md.deps) != 0 {
panic("CopyFrom requires empty destination")
}
md.schemas = append(md.schemas, from.schemas...)
md.cols = append(md.cols, from.cols...)
md.tables = append(md.tables, from.tables...)

// Clear table annotations. These objects can be mutable and can't be safely
// shared between different metadata instances.
for i := range md.tables {
md.tables[i].clearAnnotations()
}

md.sequences = append(md.sequences, from.sequences...)
md.deps = append(md.deps, from.deps...)
}

Expand Down
13 changes: 13 additions & 0 deletions pkg/sql/opt/table_meta.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ func (t TableID) index() int {
// metadata, which can be used to avoid recalculating base table properties or
// other information each time it's needed.
//
// WARNING! When copying memo metadata (which happens when we use a cached
// memo), the annotations are cleared. Any code using a annotation must treat
// this as a best-effort cache and be prepared to repopulate the annotation as
// necessary.
//
// To create a TableAnnID, call NewTableAnnID during Go's program initialization
// phase. The returned TableAnnID never clashes with other annotations on the
// same table. Here is a usage example:
Expand Down Expand Up @@ -128,6 +133,14 @@ type TableMeta struct {
anns [maxTableAnnIDCount]interface{}
}

// clearAnnotations resets all the table annotations; used when copying a
// Metadata.
func (tm *TableMeta) clearAnnotations() {
for i := range tm.anns {
tm.anns[i] = nil
}
}

// IndexColumns returns the metadata IDs for the set of columns in the given
// index.
// TODO(justin): cache this value in the table metadata.
Expand Down

0 comments on commit c1401e0

Please sign in to comment.