Do not send field query when using reserved connection

[frouioui]

Description

This Pull Request changes the query engine so it does not send the field query for SELECT statement when we are in a reserved connection. This is done so we can use the same reserved connection as the actual query when sending the field query, and thus, use the same system variables, like sql_mode for instance.

Checklist

• "Backport me!" label has been added if this change should be backported
• Tests were added or are not required
• Documentation was added or is not required

[harshit-gangal]

I think we should benchmark this change to see the impact of not catching fields.

[frouioui]

@harshit-gangal I added a benchmark in 2434d48.

The results without the change are:

goos: darwin
goarch: amd64
pkg: vitess.io/vitess/go/test/endtoend/vtgate/reservedconn
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkReservedConnFieldQuery-16          1230           1284618 ns/op
BenchmarkReservedConnFieldQuery-16           790           1292526 ns/op
BenchmarkReservedConnFieldQuery-16           984           1366862 ns/op
BenchmarkReservedConnFieldQuery-16           807           1613920 ns/op
BenchmarkReservedConnFieldQuery-16           412           2968577 ns/op
BenchmarkReservedConnFieldQuery-16           586           2412719 ns/op
BenchmarkReservedConnFieldQuery-16           478           2917387 ns/op
BenchmarkReservedConnFieldQuery-16           494           2634667 ns/op
BenchmarkReservedConnFieldQuery-16           463           2602573 ns/op
BenchmarkReservedConnFieldQuery-16           572           2096607 ns/op
PASS

And here are the results with the change:

goos: darwin
goarch: amd64
pkg: vitess.io/vitess/go/test/endtoend/vtgate/reservedconn
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkReservedConnFieldQuery-16           540           2279100 ns/op
BenchmarkReservedConnFieldQuery-16           415           2712610 ns/op
BenchmarkReservedConnFieldQuery-16           459           2423340 ns/op
BenchmarkReservedConnFieldQuery-16           454           2336300 ns/op
BenchmarkReservedConnFieldQuery-16           530           2026756 ns/op
BenchmarkReservedConnFieldQuery-16           572           1803834 ns/op
BenchmarkReservedConnFieldQuery-16           584           1748262 ns/op
BenchmarkReservedConnFieldQuery-16           613           1816961 ns/op
BenchmarkReservedConnFieldQuery-16           589           1713665 ns/op
BenchmarkReservedConnFieldQuery-16           614           1758979 ns/op
PASS

And here is the benchcmp:

benchmark                              old ns/op     new ns/op     delta
BenchmarkReservedConnFieldQuery-16     1284618       2279100       +77.41%
BenchmarkReservedConnFieldQuery-16     1292526       2712610       +109.87%
BenchmarkReservedConnFieldQuery-16     1366862       2423340       +77.29%
BenchmarkReservedConnFieldQuery-16     1613920       2336300       +44.76%
BenchmarkReservedConnFieldQuery-16     2968577       2026756       -31.73%
BenchmarkReservedConnFieldQuery-16     2412719       1803834       -25.24%
BenchmarkReservedConnFieldQuery-16     2917387       1748262       -40.07%
BenchmarkReservedConnFieldQuery-16     2634667       1816961       -31.04%
BenchmarkReservedConnFieldQuery-16     2602573       1713665       -34.15%
BenchmarkReservedConnFieldQuery-16     2096607       1758979       -16.10%

The average delta is +13.1%, so from these observations, the new code path is about 13.1% slower.

[harshit-gangal]

I think we should block those sql_modes or system variables that can lead to different fields (with and without reserved connection) so that we can cache it.

[frouioui]

@harshit-gangal, the micro-benchmark comparison between A and B, with A being this PR without the reusable reserved connections (f38bc2a) and the unsupported sql_mode check (a67b92d), and B being this PR's HEAD, are as follows:

benchmark                              old ns/op     new ns/op     delta
BenchmarkReservedConnFieldQuery-16     2127361       1944415       -8.60%
BenchmarkReservedConnFieldQuery-16     1416054       1292569       -8.72%
BenchmarkReservedConnFieldQuery-16     1341221       1306599       -2.58%
BenchmarkReservedConnFieldQuery-16     1449124       1272365       -12.20%
BenchmarkReservedConnFieldQuery-16     1479808       1302996       -11.95%
BenchmarkReservedConnFieldQuery-16     1370051       1394855       +1.81%
BenchmarkReservedConnFieldQuery-16     1408754       1553413       +10.27%
BenchmarkReservedConnFieldQuery-16     1914588       1691912       -11.63%
BenchmarkReservedConnFieldQuery-16     1271019       1484090       +16.76%
BenchmarkReservedConnFieldQuery-16     1263342       1493031       +18.18%

Overall there is not a lot of difference, the results are pretty similar.

[frouioui]

I re-ran another set of benchmarks on a more stable hardware (rather than on my MacBook).

Comparison between: before the feature and after the feature:

name                       old time/op  new time/op  delta
ReservedConnFieldQuery-48  2.13ms ± 4%  2.16ms ± 3%  +1.26%  (p=0.039 n=19+18)

Comparison between: with the feature and with feature + unauthorized sql_mode and reusable reserved connections:

name                       old time/op  new time/op  delta
ReservedConnFieldQuery-48  2.16ms ± 3%  2.11ms ± 6%  -2.53%  (p=0.006 n=18+20)

The p-value that we got for both comparisons seems low enough to me to confirm that this change is not negatively impacting the performance of Vitess.

[harshit-gangal]

This is good work Florent!

[harshit-gangal]

one nit, otherwise looks good.

[derekperkins]

Any reason not to merge this? I'm waiting on #10139, which depends on this PR

[frouioui]

Any reason not to merge this? I'm waiting on #10139, which depends on this PR

Flaky tests have been re-run, waiting on them to finish, I will then merge it and update #10139

[derekperkins]

Thanks!