Skip to content

vtcombo: Fix race condition in StreamExecute.#1323

Merged
enisoc merged 1 commit intovitessio:masterfrom
enisoc:vtcombo
Nov 15, 2015
Merged

vtcombo: Fix race condition in StreamExecute.#1323
enisoc merged 1 commit intovitessio:masterfrom
enisoc:vtcombo

Conversation

@enisoc
Copy link
Member

@enisoc enisoc commented Nov 15, 2015

@alainjobart @sougou

While porting tests from java_vtgate_test_helper to vtcombo, I found
that streaming queries that fetched more than ~300 rows would indicate
success, but receive fewer rows than expected. The number of rows actually
returned was always in the same ballpark, but would fluctuate from run
to run.

It turned out that since vtcombo returns results in-memory from vttablet
to vtgate, we were returning a *Result once the rows filled up the
buffer size, and then concurrently modifying the already-returned struct
to fill in the next set of rows.

While porting tests from java_vtgate_test_helper to vtcombo, I found
that streaming queries that fetched more than ~300 rows would indicate
success, but receive fewer rows than expected. The number of rows actually
returned was always in the same ballpark, but would fluctuate from run
to run.

It turned out that since vtcombo returns results in-memory from vttablet
to vtgate, we were returning a *Result once the rows filled up the
buffer size, and then concurrently modifying the already-returned struct
to fill in the next set of rows.
@alainjobart
Copy link
Contributor

LGTM great find!

@sougou
Copy link
Contributor

sougou commented Nov 15, 2015

lgtm for now.
I think Result should be a read-only data structure. Previously, it was not enforceable because it had to have public members (for encoding). But now, sqltypes.Result could enforce it.
OTOH, we're also switching to use querypb.QueryResult, which will again make it unenforceable in those places.
But I still think we'll get better mileage if the structs were read-only.

@enisoc
Copy link
Member Author

enisoc commented Nov 15, 2015

For Result to have true immutable semantics, you'd have to incur an extra copy of all row data upon sending to the callback:

https://github.com/youtube/vitess/blob/master/go/vt/dbconnpool/connection.go#L89

enisoc added a commit that referenced this pull request Nov 15, 2015
vtcombo: Fix race condition in StreamExecute.
@enisoc enisoc merged commit a19fefd into vitessio:master Nov 15, 2015
@enisoc enisoc deleted the vtcombo branch November 15, 2015 19:08
@sougou
Copy link
Contributor

sougou commented Nov 16, 2015

Right. We may not want to get that far. I was looking at trade-offs that will let us avoid common pitfalls without compromising performance.

notfelineit pushed a commit to planetscale/vitess that referenced this pull request Nov 16, 2022
Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants