sql: protect WaitGroup decrement in CopyIn via sync.Once #115712

yuzefovich · 2023-12-06T18:17:23Z

We've recently seen "negative WaitGroup counter" server crash during COPY FROM execution a few times, but we have been unable to understand the root cause. It appears that the problem can happen right after the COPY execution is canceled due to statement_timeout. The synchronization setup is the following:

the network-handling goroutine calls wg.Add(1), pushes CopyIn command onto the stmt buf, and then blocks via wg.Wait()
the copy-handling connExecutor calls wg.Done() in the defer of execCopyIn. It must be the case that that defer is executed at least twice, but it's unclear to me how that can happen.

In the absence of understanding of how this can happen and with no reproduction, this commit attempts to mitigate the problem by ensuring that wg.Done() is called exactly once. This is achieved via sync.Once.

Fixes: #112095.

Release note: None

blathers-crl · 2023-12-06T18:17:27Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-12-06T18:17:32Z

This change is

yuzefovich · 2023-12-06T18:18:36Z

I'm not very happy with this change, but I don't have any other ideas on how to mitigate #112095 / https://github.com/cockroachlabs/support/issues/2741. Curious about thoughts on whether it's worth making this change or not.

rafiss · 2023-12-06T19:26:16Z

pkg/sql/conn_executor.go

@@ -3006,7 +3006,9 @@ func (ex *connExecutor) execCopyIn(
 	}()

 	// When we're done, unblock the network connection.
-	defer cmd.CopyDone.Done()
+	defer func() {


one more idea instead of a buffered channel: could we change this to something like:

once.Do(func() { close(cmd.CopyDone) })

or

once.Do(func() { cmd.CopyDone.Done() })

yuzefovich

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @rafiss)

pkg/sql/conn_executor.go line 3009 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

one more idea instead of a buffered channel: could we change this to something like:
once.Do(func() { close(cmd.CopyDone) })
or
once.Do(func() { cmd.CopyDone.Done() })

I like it, thanks, done.

rafiss

lgtm. ty for fixing!

rafiss · 2023-12-07T02:36:20Z

pkg/sql/conn_io.go

@@ -370,6 +370,8 @@ type CopyIn struct {
 	// CopyDone is decremented once execution finishes, signaling that control of
 	// the connection is being handed back to the network routine.
 	CopyDone *sync.WaitGroup
+	// Once is used to decrement CopyDone exactly once.
+	Once *sync.Once


one thought i had is if this would be more clear with an embedded struct:

CopyDone struct { *sync.WaitGroup *sync.Once }

(so that it's obvious the two should be used together

We've recently seen "negative WaitGroup counter" server crash during COPY FROM execution a few times, but we have been unable to understand the root cause. It appears that the problem can happen right after the COPY execution is canceled due to `statement_timeout`. The synchronization setup is the following: - the network-handling goroutine calls `wg.Add(1)`, pushes CopyIn command onto the stmt buf, and then blocks via `wg.Wait()` - the copy-handling connExecutor calls `wg.Done()` in the defer of `execCopyIn`. It must be the case that that defer is executed at least twice, but it's unclear to me how that can happen. In the absence of understanding of how this can happen and with no reproduction, this commit attempts to mitigate the problem by ensuring that `wg.Done()` is called exactly once. This is achieved via `sync.Once`. Release note: None

yuzefovich

TFTR!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @rafiss)

pkg/sql/conn_io.go line 374 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

one thought i had is if this would be more clear with an embedded struct:
	CopyDone struct {
		*sync.WaitGroup
		*sync.Once
	}
(so that it's obvious the two should be used together

Very nice, done.

craig · 2023-12-07T19:01:52Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-12-07T19:02:09Z

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

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

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

error setting reviewers, but backport branch blathers/backport-release-23.1-115712 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/115799/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. []

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

error setting reviewers, but backport branch blathers/backport-release-23.2-115712 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/115800/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. []

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

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

yuzefovich · 2023-12-07T21:24:59Z

blathers backport release-23.2.0

blathers-crl · 2023-12-07T21:25:10Z

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

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

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

error getting backport branch release-release-23.2.0: unexpected status code: 404 Not Found

Backport to branch release-23.2.0 failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

yuzefovich · 2023-12-07T21:29:22Z

blathers backport 23.2.0

blathers-crl · 2023-12-07T21:29:24Z

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

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

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

error getting backport branch release-23.2.0: unexpected status code: 404 Not Found

Backport to branch 23.2.0 failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

yuzefovich · 2023-12-07T21:29:47Z

blathers backport 23.2.0-rc

blathers-crl · 2023-12-07T21:29:55Z

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

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

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

error setting reviewers, but backport branch blathers/backport-release-23.2.0-rc-115712 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/115829/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. []

Backport to branch 23.2.0-rc failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

yuzefovich added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. labels Dec 6, 2023

yuzefovich requested review from rafiss and michae2 December 6, 2023 18:17

yuzefovich requested review from a team as code owners December 6, 2023 18:17

yuzefovich force-pushed the copy-wg branch from 7ea887c to 396f7b2 Compare December 6, 2023 18:21

yuzefovich mentioned this pull request Dec 6, 2023

sql: prevent crash with COPY FROM in an edge case #115720

Closed

rafiss reviewed Dec 6, 2023

View reviewed changes

yuzefovich force-pushed the copy-wg branch from 396f7b2 to 9fb1d63 Compare December 6, 2023 19:57

yuzefovich changed the title ~~sql: replace WaitGroup in CopyIn with a channel~~ sql: protect WaitGroup decrement in CopyIn via sync.Once Dec 6, 2023

yuzefovich commented Dec 6, 2023

View reviewed changes

rafiss approved these changes Dec 7, 2023

View reviewed changes

yuzefovich force-pushed the copy-wg branch from 9fb1d63 to d017766 Compare December 7, 2023 17:15

yuzefovich requested a review from rafiss December 7, 2023 17:16

yuzefovich commented Dec 7, 2023

View reviewed changes

rafiss approved these changes Dec 7, 2023

View reviewed changes

craig bot merged commit af1fda5 into cockroachdb:master Dec 7, 2023
8 of 9 checks passed

This was referenced Dec 7, 2023

release-23.1: sql: protect WaitGroup decrement in CopyIn via sync.Once #115799

Merged

release-23.2: sql: protect WaitGroup decrement in CopyIn via sync.Once #115800

Merged

yuzefovich deleted the copy-wg branch December 7, 2023 20:30

blathers-crl bot mentioned this pull request Dec 7, 2023

release-23.2.0-rc: sql: protect WaitGroup decrement in CopyIn via sync.Once #115829

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: protect WaitGroup decrement in CopyIn via sync.Once #115712

sql: protect WaitGroup decrement in CopyIn via sync.Once #115712

yuzefovich commented Dec 6, 2023 •

edited

Loading

blathers-crl bot commented Dec 6, 2023

cockroach-teamcity commented Dec 6, 2023

yuzefovich commented Dec 6, 2023

rafiss Dec 6, 2023 •

edited

Loading

yuzefovich left a comment

rafiss left a comment

rafiss Dec 7, 2023

yuzefovich left a comment

craig bot commented Dec 7, 2023

blathers-crl bot commented Dec 7, 2023

yuzefovich commented Dec 7, 2023

blathers-crl bot commented Dec 7, 2023

yuzefovich commented Dec 7, 2023

blathers-crl bot commented Dec 7, 2023

yuzefovich commented Dec 7, 2023

blathers-crl bot commented Dec 7, 2023

sql: protect WaitGroup decrement in CopyIn via sync.Once #115712

sql: protect WaitGroup decrement in CopyIn via sync.Once #115712

Conversation

yuzefovich commented Dec 6, 2023 • edited Loading

blathers-crl bot commented Dec 6, 2023

cockroach-teamcity commented Dec 6, 2023

yuzefovich commented Dec 6, 2023

rafiss Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss Dec 7, 2023

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

craig bot commented Dec 7, 2023

blathers-crl bot commented Dec 7, 2023

yuzefovich commented Dec 7, 2023

blathers-crl bot commented Dec 7, 2023

yuzefovich commented Dec 7, 2023

blathers-crl bot commented Dec 7, 2023

yuzefovich commented Dec 7, 2023

blathers-crl bot commented Dec 7, 2023

yuzefovich commented Dec 6, 2023 •

edited

Loading

rafiss Dec 6, 2023 •

edited

Loading