opt: fix panic due to concurrent map writes when copying metadata#66792
opt: fix panic due to concurrent map writes when copying metadata#66792craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
michae2
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @RaduBerinde)
RaduBerinde
left a comment
There was a problem hiding this comment.
Oof, LGTM
Maybe we should tighten our policy for how soon non-critical backports can be merged. In this case we would have needed ~3 days.
Reviewable status:
|
TFTRs!
Yea, good call bors r+ |
|
Looks like I need to update a test bors r- |
|
Canceled. |
94fbb9d to
75569a9
Compare
|
Fixed -- PTAL |
michae2
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status:
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/table_meta.go, line 190 at r2 (raw file):
} if len(tm.ComputedCols) > 0 {
I think this should be if tm.ComputedCols == nil. We don't want to share the map if it happens to be empty. (I realize an empty map probably never happens in practice but still)
This commit fixes a bug that caused a panic due to concurrent map writes when copying table metadata. The fix is to make a deep copy of the map before updating it. Fixes cockroachdb#66717 Release note (bug fix): Fixed a panic that could occur in the optimizer when executing a prepared plan with placeholders. This could happen when one of the tables used by the query had computed columns or a partial index.
75569a9 to
990807e
Compare
rytaft
left a comment
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
pkg/sql/opt/table_meta.go, line 190 at r2 (raw file):
Previously, RaduBerinde wrote…
I think this should be
if tm.ComputedCols == nil. We don't want to share the map if it happens to be empty. (I realize an empty map probably never happens in practice but still)
Done.
|
Build failed (retrying...): |
|
Oh ok thanks, @tbg. I don't want to stop bors at this point, but I will close/reopen the issues after. |
|
Build succeeded: |
|
😬 Thanks for fixing this! |
…ata maps This commit adds assertions to `TestMetadata` that ensure that `Metadata.CopyFrom` makes copies of computed column expression maps and parital index predicate maps in table metadata, rather than sharing the same underlying map. These tests would have caught the bug that was fixed in cockroachdb#66792. Release note: None
66595: ui: add an Overload dashboard r=RaduBerinde a=RaduBerinde This commit adds an Overload dashboard in the metrics view. This is intended to be a convenient way to monitor admission control. The dashboard contains: - SQL Queries - Service latency - CPU Percent - Runnable Goroutines per CPU - L0 Sublevels and Files Release note (ui change): a new Overload dashboard groups metrics that are useful for admission control. 66786: sql: incorporate lookupExpr cpu cost into cost model r=rytaft a=cucaroach Informs #51576 This is a prep-the-patient change required for enabling the lookup join inequality conditions. It enables for instance partial indexes to be selected when there's a costing tie between a lookup join with a partial index vs a full index with the partial index qualifier as a lookup join condition. Release note (sql): improve cost model to include cpu cost of lookupExprs used by lookup joins. 67078: opt: add test to ensure Metadata.CopyFrom makes copies of table metadata maps r=mgartner a=mgartner This commit adds assertions to `TestMetadata` that ensure that `Metadata.CopyFrom` makes copies of computed column expression maps and parital index predicate maps in table metadata, rather than sharing the same underlying map. These tests would have caught the bug that was fixed in #66792. Release note: None Co-authored-by: Radu Berinde <radu@cockroachlabs.com> Co-authored-by: Tommy Reilly <treilly@cockroachlabs.com> Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
This commit fixes a bug that caused a panic due to concurrent map writes
when copying table metadata. The fix is to make a deep copy of the map
before updating it.
Fixes #66717
Release note (bug fix): Fixed a panic that could occur in the optimizer
when executing a prepared plan with placeholders. This could happen when
one of the tables used by the query had computed columns or a partial
index.