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.

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

sql: panic: concurrent map read and map write with parallel statements #23171

Closed
jordanlewis opened this issue Feb 27, 2018 · 3 comments · Fixed by #26670
Closed

sql: panic: concurrent map read and map write with parallel statements #23171

jordanlewis opened this issue Feb 27, 2018 · 3 comments · Fixed by #26670
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting.
Milestone

Comments

@jordanlewis
Copy link
Member

Seen when running a local tpcc with @nvanbenschoten's recent RETURNING NOTHING changes.

fatal error: concurrent map read and map write

goroutine 56464 [running]:
runtime.throw(0x5dbf65a, 0x21)
        /usr/local/go/src/runtime/panic.go:616 +0x81 fp=0xc4273ab5a0 sp=0xc4273ab580 pc=0x402fdf1
runtime.mapaccess2_faststr(0x5b5a5c0, 0xc42901e060, 0xc428806654, 0x1, 0xc42d826748, 0x7218501)
        /usr/local/go/src/runtime/hashmap_fast.go:270 +0x461 fp=0xc4273ab610 sp=0xc4273ab5a0 pc=0x40101a1
github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*PlaceholderInfo).Type(...)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/sem/tree/placeholders.go:128
github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*Placeholder).Eval(0xc42ef79f40, 0xc424c21570, 0xc42ef79f40, 0x6080d20, 0xc42ef79f40, 0x0)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/sem/tree/eval.go:3662 +0xf1 fp=0xc4273ab700 sp=0xc4273ab610 pc=0x4834f01
github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*BinaryExpr).Eval(0xc426281b90, 0xc424c21570, 0x6094980, 0xc4262782a0, 0x0, 0x0)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/sem/tree/eval.go:2540 +0xfc fp=0xc4273ab7c8 sp=0xc4273ab700 pc=0x482ad7c
github.com/cockroachdb/cockroach/pkg/sql.(*renderNode).renderRow(0xc426502c60, 0xc424c21570, 0x15, 0x15)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/render.go:697 +0xba fp=0xc4273ab828 sp=0xc4273ab7c8 pc=0x50d3d5a
github.com/cockroachdb/cockroach/pkg/sql.(*renderNode).Next(0xc426502c60, 0x60747e0, 0xc425fe2080, 0xc424c21570, 0xc424c21500, 0x0, 0x0, 0x0)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/render.go:344 +0xc7 fp=0xc4273ab878 sp=0xc4273ab828 pc=0x50d1747
github.com/cockroachdb/cockroach/pkg/sql.(*updateNode).Next(0xc427446580, 0x60747e0, 0xc425fe2080, 0xc424c21570, 0xc424c21500, 0x5bbd660, 0xc427446501, 0x0)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/update.go:304 +0x79 fp=0xc4273abc18 sp=0xc4273ab878 pc=0x51272d9
github.com/cockroachdb/cockroach/pkg/sql.forEachRow(0x60747e0, 0xc425fe2080, 0xc424c21570, 0xc424c21500, 0x606b4a0, 0xc427446580, 0xc4273abcb8, 0xc424c21500, 0x606b4a0)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/executor.go:2021 +0x5d fp=0xc4273abc68 sp=0xc4273abc18 pc=0x5073c0d
github.com/cockroachdb/cockroach/pkg/sql.countRowsAffected(0x60747e0, 0xc425fe2080, 0xc424c21570, 0xc424c21500, 0x606b4a0, 0xc427446580, 0x0, 0x409b226, 0x5a95a962)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/executor.go:2045 +0xd7 fp=0xc4273abcd8 sp=0xc4273abc68 pc=0x5073e07
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execWithLocalEngine(0xc424c9c000, 0x60747e0, 0xc425fe2080, 0xc424c21500, 0x2, 0x6085640, 0xc42ae81480, 0x0, 0x0)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:580 +0x2d7 fp=0xc4273abd80 sp=0xc4273abcd8 pc=0x5017737
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmtInParallel.func1(0x0, 0x0)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:446 +0x2d5 fp=0xc4273abf18 sp=0xc4273abd80 pc=0x5152835
github.com/cockroachdb/cockroach/pkg/sql.(*ParallelizeQueue).Add.func1(0xc424c9ceb0, 0x60747e0, 0xc425fe2080, 0xc424c21570, 0xc424c21500, 0xc425ac0070, 0x2, 0x2, 0xc4262cdc20, 0xc426281dd0)
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/parallel_stmts.go:118 +0x111 fp=0xc4273abf90 sp=0xc4273abf18 pc=0x515dfc1
runtime.goexit()
        /usr/local/go/src/runtime/asm_amd64.s:2361 +0x1 fp=0xc4273abf98 sp=0xc4273abf90 pc=0x405e3f1
created by github.com/cockroachdb/cockroach/pkg/sql.(*ParallelizeQueue).Add
        /Users/jordan/repo/src/github.com/cockroachdb/cockroach/pkg/sql/parallel_stmts.go:89 +0x1cc```
@jordanlewis
Copy link
Member Author

cc @andreimatei

I believe the problem here is that a pgwire PreparedPortal has a single underlying map of placeholder types that gets passed to the EvalContext and SemaContext of a running query.

When executing a prepared statement that can be parallelized, the PreparedPortal's placeholder type map is sent to the ParallelizeQueue as normal, without copying. If the execution of that statement modifies the placeholder type map (which happens when there's not a well-known type for a placeholder until a concrete value is sent at execute time) at the same time as another parallel statement reads it, we get this panic.

Since it's valid for execution to modify the portal's map, I think we need to copy the map when doing parallel execution.

Side note - we actually might need to copy the map when doing normal execution too, or else the first execution of a prepared statement with ambiguous placeholder types might possibly type check differently than the second execution. This latter case seems difficult to construct and unimportant in practice though.

@knz
Copy link
Contributor

knz commented Feb 28, 2018

the first execution of a prepared statement with ambiguous placeholder types might possibly type check differently than the second execution

Similar to #22847

@jordanlewis
Copy link
Member Author

Similar, but thankfully easier to fix. This doesn't mutate the AST or anything. It modifies the values in EvalContext, which seems much more valid to me.

@petermattis petermattis modified the milestones: 2.0, 2.0.x Apr 5, 2018
@knz knz changed the title panic: concurrent map read and map write with parallel statements sql: panic: concurrent map read and map write with parallel statements Apr 17, 2018
@knz knz added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Apr 17, 2018
@jordanlewis jordanlewis added the S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting. label Apr 24, 2018
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Jun 13, 2018
Fixes cockroachdb#23171.

This change fixes a `concurrent map read and map write` panic.
The panic was caused because a shared map that is populated during
statement preparation was being updated during statement execution.
However, this update was unnecessary because we were simply replacing
a value with itself. This change fixes the issue.

Release note: None
craig bot pushed a commit that referenced this issue Jun 13, 2018
26670: sql: only set PlaceholderInfo type if not already set r=nvanbenschoten a=nvanbenschoten

Fixes #23171.

This change fixes a `concurrent map read and map write` panic.
The panic was caused because a shared map that is populated during
statement preparation was being updated during statement execution.
However, this update was unnecessary because we were simply replacing
a value with itself. This change fixes the issue.

Release note: None

Co-authored-by: Nathan VanBenschoten <[email protected]>
@craig craig bot closed this as completed in #26670 Jun 13, 2018
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Jul 10, 2018
Fixes cockroachdb#23171.

This change fixes a `concurrent map read and map write` panic.
The panic was caused because a shared map that is populated during
statement preparation was being updated during statement execution.
However, this update was unnecessary because we were simply replacing
a value with itself. This change fixes the issue.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants