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

changefeedccl: avoid concurrent map access #88130

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

HonoreDB
Copy link
Contributor

go 1.18 introduced more stringent checks for unsafe concurrent map use, surfacing some new and exciting panics in changefeed code.

When backported, fixes #87939
When backported, fixes #88089
When backported, fixes #87899

Release note (bug fix): Fixed crashes in changefeed code when running on recent go versions.

@HonoreDB HonoreDB requested a review from miretskiy September 19, 2022 14:36
@HonoreDB HonoreDB requested a review from a team as a code owner September 19, 2022 14:36
@cockroach-teamcity
Copy link
Member

This change is Reviewable

go 1.18 introduced more stringent checks for unsafe concurrent map use, surfacing
some new and exciting panics in changefeed code.

When backported, fixes cockroachdb#87939
When backported, fixes cockroachdb#88089
When backported, fixes cockroachdb#87899

Release note (bug fix): Fixed crashes in changefeed code when running on recent go versions.
@HonoreDB HonoreDB force-pushed the fix_concurrent_map_access branch from 879402a to 03926b3 Compare September 19, 2022 18:16
@HonoreDB
Copy link
Contributor Author

bors r=[miretskiy]

@craig
Copy link
Contributor

craig bot commented Sep 19, 2022

Build succeeded:

@craig craig bot merged commit 7bde044 into cockroachdb:master Sep 19, 2022
@blathers-crl
Copy link

blathers-crl bot commented Sep 19, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 03926b3 to blathers/backport-release-22.1-88130: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.


error creating merge commit from 03926b3 to blathers/backport-release-22.2-88130: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

HonoreDB added a commit to HonoreDB/cockroach that referenced this pull request Sep 20, 2022
cockroachdb#88130
introduced a deadlock when an attempt to create a
topic fails -- the goroutine tries to acquire a lock
in order to record the error, but it already has it
in order to write to the map. This PR releases the lock
while creating the topic, which should also help with
performance a bit on startup.

Release note (bug fix): Fixed a bug preventing pubsub changefeeds from retrying.
HonoreDB added a commit to HonoreDB/cockroach that referenced this pull request Sep 21, 2022
cockroachdb#88130
introduced a deadlock when an attempt to create a
topic fails -- the goroutine tries to acquire a lock
in order to record the error, but it already has it
in order to write to the map. This PR releases the lock
while creating the topic, which should also help with
performance a bit on startup.

Release note (bug fix): Fixed a bug preventing pubsub changefeeds from retrying.
HonoreDB added a commit to HonoreDB/cockroach that referenced this pull request Sep 21, 2022
cockroachdb#88130
introduced a deadlock when an attempt to create a
topic fails -- the goroutine tries to acquire a lock
in order to record the error, but it already has it
in order to write to the map. This PR releases the lock
while creating the topic, which should also help with
performance a bit on startup.

Release note (bug fix): Fixed a bug preventing pubsub changefeeds from retrying.
craig bot pushed a commit that referenced this pull request Sep 27, 2022
88076: admission: Split stats for WorkQueueMetrics r=irfansharif a=andrewbaptist

This commit splits the WorkQueueMetric stats into priority.
Because there are a lot of new stats that might be generated
as a result of this splitting, only the "important" priorities
are collected and considered for each queue.

Release note: None

88289: cdc: avoid deadlock on error in pubsub sink r=HonoreDB a=HonoreDB

#88130 introduced a deadlock when an attempt to create a
topic fails -- the goroutine tries to acquire a lock in order to record the error, but it already has it in order to write to the map. This PR releases the lock while creating the topic, which should also help with performance a bit on startup.

Fixes #85374

Release note (bug fix): Fixed a bug preventing pubsub changefeeds from retrying.

88491: opt: do not plan unnecessary paired semi- and anti- lookup joins r=mgartner a=mgartner

This commit fixes an issue where the optimizer would plan a paired semi
or anti lookup join in cases when a single lookup join would suffice.
This only occurred in rare cases when the join filter contained a
tautology or contradiction that could not be normalized to true or false
in the canonical query plan, but could be eliminated from the filters
when building a lookup join. If the tautology or contradiction
referenced a column not covered by the lookup index, the optimizer
mistakenly assumed that the index was not covering and planned a paired
join. Now the optimizer can recognize that the index is actually
covering, because the referenced column is not needed to evaluate the
filters, and a single lookup join is planned.

Fixes #87306

Release note (performance improvement): The optimizer now explores plans
with a single lookup join expressions in rare cases where it previously
planned two lookup join expressions.


88714: ttl: test multiple TTL storage param changes r=rafiss,ajwerner a=ecwall

fixes #88560

Add tests to cover multiple changes to TTL storage params.

Release note: None

88756: sql: only record index reads on non-internal sessions r=THardy98 a=THardy98

Resolves: #77598

Previously, explicit `CREATE INDEX` queries were causing unintended index reads in our `index_usage_statistics` subsystem. The unintended index reads were caused by an index validation query from an internal executor that did not use an internal planner. However, the internal executor does set its session data as internal. This change adds a check to only record index reads when the planner is not internal AND the session is not internal.

Release note (bug fix): Fixed unintended recordings of index reads caused by internal executor/queries.

88773: sql: support `SHOW GRANTS` for functions r=chengxiong-ruan a=chengxiong-ruan

Backport resolves #88495

Release note (sql change): Previously `SHOW GRANTS` only supports
db, schema, table and types. This commit add supports for UDFs,
so that `SHOW GRANTS` returns UDFs privileges infos, and statements
like `SHOW GRANTS ON FUNCTION <udf name/signatures>` are now supported
Full function signature must be provided if the function name is
not unique.
Release justification: low risk GA blocker.

88831: bench: add a benchmark for INSERT RETURNING r=yuzefovich a=yuzefovich

Informs #88525.

Release note: None

Co-authored-by: Andrew Baptist <[email protected]>
Co-authored-by: Aaron Zinger <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: Thomas Hardy <[email protected]>
Co-authored-by: Chengxiong Ruan <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
blathers-crl bot pushed a commit that referenced this pull request Sep 27, 2022
#88130
introduced a deadlock when an attempt to create a
topic fails -- the goroutine tries to acquire a lock
in order to record the error, but it already has it
in order to write to the map. This PR releases the lock
while creating the topic, which should also help with
performance a bit on startup.

Release note (bug fix): Fixed a bug preventing pubsub changefeeds from retrying.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants