vtexplain: Fix setting up the column information by dbussink · Pull Request #15275 · vitessio/vitess

dbussink · 2024-02-17T14:17:55Z

In #13312 we fixed how we encode names in information_schema queries. The problem there though is that the GetColumnNamesQuery string was also used inside vtexplain. Its usage there was not updated for the new escaping logic.

This in turn leads to broken queries during vtexplain which then leads to a lot of logs. The issue here is not that we log a lot, we log that we actually do have a problem here which is the case.

This fixes the underlying issue of not correctly generating the query as we should.

Related Issue(s)

Part of #15242

Marked for backporting all the way to v16, since that's where the original change in #13312 was also back ported to.

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

In vitessio#13312 we fixed how we encode names in information_schema queries. The problem there though is that the `GetColumnNamesQuery` string was also used inside `vtexplain`. It's usage there was not updated for the new escaping logic. This in turn leads to broken queries during `vtexplain` which then leads to a lot of logs. The issue here is not that we log a lot, we log that we actually do have a problem here which is the case. This fixes the underlying issue of not correctly generating the query as we should. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

vitess-bot · 2024-02-17T14:17:57Z

dbussink · 2024-02-17T14:18:38Z

go/vt/vtexplain/vtexplain_vttablet.go

 	// Gen4 supports more complex queries so we now need to
 	// handle multiple FROM clauses
-	tables := make([]*sqlparser.AliasedTableExpr, len(selStmt.From))
+	tables := make([]*sqlparser.AliasedTableExpr, 0, len(selStmt.From))


Found this bug as well while debugging. We create it with the length, which means subsequent append below would always create an even longer slice, negating this optimization plus having a lot of nil entries in this slice.

It's really a golang design flaw. I've fallen for this many times.

GrahamCampbell · 2024-02-17T14:22:44Z

go/vt/vtexplain/vtexplain_vttablet.go


-			likeQuery = fmt.Sprintf(mysqlctl.GetColumnNamesQuery, "database()", sanitizedLikeTable)
-			query = fmt.Sprintf(mysqlctl.GetColumnNamesQuery, "database()", sanitizedTable)
+			likeQuery = fmt.Sprintf(mysqlctl.GetColumnNamesQuery, "database()", mysqlctl.EncodeEntityName(sanitizedLikeTable))


Is it correct to apply the encoding onto the like expression? It's not a table name.

Yes, this creates a correctly escaped string from the input which is what we want here explicitly.

codecov · 2024-02-17T14:35:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (54c6dfc) 67.44% compared to head (4f04f20) 67.44%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15275      +/-   ##
==========================================
- Coverage   67.44%   67.44%   -0.01%     
==========================================
  Files        1561     1561              
  Lines      193193   193189       -4     
==========================================
- Hits       130304   130296       -8     
- Misses      62889    62893       +4

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

mattlord

LGTM. Can we add/update a test to catch things like this in the future?

dbussink · 2024-02-17T15:39:43Z

LGTM. Can we add/update a test to catch things like this in the future?

I don't really have a good idea for how to add one. We only seem to log the errors, not fail on them. I wonder if we should fail on them instead but I don't really know enough about vtexplain if that's desirable here or not.

@vitessio/query-serving Does anyone of you have an idea how to test for this case here?

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

dbussink · 2024-02-18T14:30:52Z

@mattlord Added a test in 241d5f5, but not really happy with it. Would not mind if others have a better idea 😄.

shlomi-noach · 2024-02-19T06:01:08Z

go/vt/vtexplain/vtexplain_vttablet.go

 	// Gen4 supports more complex queries so we now need to
 	// handle multiple FROM clauses
-	tables := make([]*sqlparser.AliasedTableExpr, len(selStmt.From))
+	tables := make([]*sqlparser.AliasedTableExpr, 0, len(selStmt.From))


It's really a golang design flaw. I've fallen for this many times.

There's already `sqltypes.EncodeStringSQL` which implements the exact same thing. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

…) (#15282) Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

…) (#15281) Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

…) (#15280) Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

Backport vitessio#15275

dbussink added Backport to: release-16.0 labels Feb 17, 2024

dbussink requested review from deepthi, harshit-gangal, mattlord and systay as code owners February 17, 2024 14:17

github-actions bot added this to the v20.0.0 milestone Feb 17, 2024

dbussink commented Feb 17, 2024

View reviewed changes

dbussink mentioned this pull request Feb 17, 2024

Suppress invalid column errors in vtexplain #15272

Closed

5 tasks

GrahamCampbell reviewed Feb 17, 2024

View reviewed changes

mattlord reviewed Feb 17, 2024

View reviewed changes

dbussink requested review from ajm188, rohit-nayak-ps and shlomi-noach as code owners February 18, 2024 14:13

dbussink force-pushed the fix-vtexplain-column-lookup-issue branch from 9c769db to 939de97 Compare February 18, 2024 14:17

Add basic test

241d5f5

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

shlomi-noach approved these changes Feb 19, 2024

View reviewed changes

dbussink removed the Backport to: release-16.0 label Feb 19, 2024

Remove unneeded duplicate function

4f04f20

There's already `sqltypes.EncodeStringSQL` which implements the exact same thing. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

systay approved these changes Feb 19, 2024

View reviewed changes

dbussink merged commit 9a78e7d into vitessio:main Feb 19, 2024

dbussink deleted the fix-vtexplain-column-lookup-issue branch February 19, 2024 10:23

This was referenced Feb 19, 2024

[release-17.0] vtexplain: Fix setting up the column information (#15275) #15280

Merged

[release-18.0] vtexplain: Fix setting up the column information (#15275) #15281

Merged

[release-19.0] vtexplain: Fix setting up the column information (#15275) #15282

Merged

vitess-bot pushed a commit that referenced this pull request Feb 19, 2024

vtexplain: Fix setting up the column information (#15275)

6f044e2

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

This was referenced Feb 19, 2024

vtexplain: Ensure memory topo is set up for throttler #15279

Merged

Bug Report: Noisy vtexplain log output #15242

Closed

hmaurer added a commit to github/vitess-gh that referenced this pull request Mar 21, 2024

Backport vitessio#15275

bab4912

This was referenced Mar 21, 2024

oops #15542

Closed

Backport https://github.com/vitessio/vitess/pull/15275 github/vitess-gh#99

Merged

hmaurer added a commit to github/vitess-gh that referenced this pull request Mar 21, 2024

Merge pull request #99 from github/hm/backport-vtexplain-fix

a8bc7c0

Backport vitessio#15275

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vtexplain: Fix setting up the column information#15275

vtexplain: Fix setting up the column information#15275
dbussink merged 3 commits intovitessio:mainfrom
dbussink:fix-vtexplain-column-lookup-issue

dbussink commented Feb 17, 2024 •

edited

Loading

Uh oh!

vitess-bot bot commented Feb 17, 2024

Uh oh!

dbussink Feb 17, 2024

Uh oh!

shlomi-noach Feb 19, 2024

Uh oh!

GrahamCampbell Feb 17, 2024

Uh oh!

dbussink Feb 17, 2024

Uh oh!

codecov bot commented Feb 17, 2024 •

edited

Loading

Uh oh!

mattlord left a comment

Uh oh!

dbussink commented Feb 17, 2024

Uh oh!

dbussink commented Feb 18, 2024

Uh oh!

shlomi-noach Feb 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

dbussink commented Feb 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issue(s)

Checklist

Uh oh!

vitess-bot bot commented Feb 17, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

Uh oh!

dbussink Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

shlomi-noach Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

GrahamCampbell Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

dbussink Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mattlord left a comment

Choose a reason for hiding this comment

Uh oh!

dbussink commented Feb 17, 2024

Uh oh!

dbussink commented Feb 18, 2024

Uh oh!

shlomi-noach Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dbussink commented Feb 17, 2024 •

edited

Loading

codecov bot commented Feb 17, 2024 •

edited

Loading