[release-19.0] Improve performance for BaseShowTablesWithSizes query. (#15713)

[vitess-bot]

Description

This is a backport of #15713

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.44%. Comparing base (67dcca1) to head (a294f60).

Additional details and impacted files

@@               Coverage Diff                @@
##           release-19.0   #15795      +/-   ##
================================================
- Coverage         67.45%   67.44%   -0.01%     
================================================
  Files              1560     1560              
  Lines            192777   192777              
================================================
- Hits             130032   130016      -16     
- Misses            62745    62761      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[arthurschreiber]

@frouioui The upgrade-downgrade tests fail because the bind variable type comment changed for decimal types.

:vtg6 /* DECIMAL(3,2) */, :vtg7 /* DECIMAL(3,2) */ is what was generated, while :vtg6 /* DECIMAL */, :vtg7 /* DECIMAL */ is what the test expects to find.

vitess/go/test/endtoend/vtgate/queries/normalize/normalize_test.go

Lines 43 to 49 in 354bff9

	normalizedInsertQuery := `insert into t1 values (:vtg1 /* INT64 */, :vtg2 /* VARCHAR */, :vtg3 /* VARCHAR */, :vtg4 /* HEXVAL */, :vtg5 /* HEXNUM */, :vtg6 /* DECIMAL */, :vtg7 /* DECIMAL */, :vtg8 /* INT64 */, :vtg9 /* VARCHAR */, :vtg10 /* VARCHAR */, :vtg11 /* VARCHAR */, :vtg12 /* VARCHAR */, point(:vtg13 /* INT64 */, :vtg14 /* INT64 */), :vtg15 /* BITNUM */, :vtg16 /* BITNUM */)`
	vtgateVersion, err := cluster.GetMajorVersion("vtgate")
	require.NoError(t, err)
	if vtgateVersion < 19 {
	normalizedInsertQuery = `insert into t1 values (:vtg1 /* INT64 */, :vtg2 /* VARCHAR */, :vtg3 /* VARCHAR */, :vtg4 /* HEXVAL */, :vtg5 /* HEXNUM */, :vtg6 /* DECIMAL */, :vtg7 /* DECIMAL */, :vtg8 /* INT64 */, :vtg9 /* VARCHAR */, :vtg10 /* VARCHAR */, :vtg11 /* VARCHAR */, :vtg12 /* VARCHAR */, point(:vtg13 /* INT64 */, :vtg14 /* INT64 */), :vtg15 /* HEXNUM */, :vtg16 /* HEXNUM */)`
	}
	selectQuery := "select * from t1"

Why does the above test set normalizedInsertQuery to the same value both on line 43 and line 47? Was some merge conflict resolved incorrect?

I'd think that probably line 43 should include the "updated" decimal comment?

[frouioui]

Was some merge conflict resolved incorrect?

There were no merge conflict, the PR was created cleanly.

Why does the above test set normalizedInsertQuery to the same value both on line 43 and line 47?

I think they are comparing different values.

line 43:  :vtg15 /* BITNUM */, :vtg16 /* BITNUM */)
line 47:  :vtg15 /* HEXNUM */, :vtg16 /* HEXNUM */)

[frouioui]

I'd think that probably line 43 should include the "updated" decimal comment?

I find it weird that we did not have to update that test on the main PR though.

[arthurschreiber]

I think they are comparing different values.

Yes, you are right. But the query collected in the test looks like this:

insert into t1 values (:vtg1 /* INT64 */, :vtg2 /* VARCHAR */, :vtg3 /* VARCHAR */, :vtg4 /* HEXVAL */, :vtg5 /* HEXNUM */, :vtg6 /* DECIMAL(3,2) */, :vtg7 /* DECIMAL(3,2) */, :vtg8 /* INT64 */, :vtg9 /* VARCHAR */, :vtg10 /* VARCHAR */, :vtg11 /* VARCHAR */, :vtg12 /* VARCHAR */, point(:vtg13 /* INT64 */, :vtg14 /* INT64 */), :vtg15 /* BITNUM */, :vtg16 /* BITNUM */)

I updated the tests, let's see if that gets us to green.

[arthurschreiber]

I might have figured this out? The generated query text changed from v18 to v19, and from v19 to v20. The upgrade - downgrade tests I guess test both combination, but only had logic to account for the query difference between v18 and v19, but not v19 and v20. I added another check and I think this should work now.

[arthurschreiber]

@frouioui can you take another look? 🙇

[frouioui]

LGTM, thank you @arthurschreiber 🙇🏻