Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query information_schema compatible with MySQL 8 #2652

Merged
merged 3 commits into from
Jan 13, 2022

Conversation

px3303
Copy link
Contributor

@px3303 px3303 commented Jan 11, 2022

Turn off statistics caching for MySQL 8.
To always retrieve the latest statistics directly from the storage engine and bypass cached values, set information_schema_stats_expiry to 0.
See https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_information_schema_stats_expiry

Fixes #2653

Checklist

Turn off statistics caching for MySQL 8.
To always retrieve the latest statistics directly from the storage engine and bypass cached values, set information_schema_stats_expiry to 0.
See https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_information_schema_stats_expiry
@px3303 px3303 requested a review from a team as a code owner January 11, 2022 14:29
@px3303 px3303 requested a review from pav-kv January 11, 2022 14:29
@AlCutter
Copy link
Member

Hi px3303, many thanks for the PR!

To help understanding the context for it, would you mind updating the description with a bit of background on why this change is necessary/what [potential] problem it fixes?

Thanks in advance.

@AlCutter AlCutter self-assigned this Jan 11, 2022
@px3303
Copy link
Contributor Author

px3303 commented Jan 12, 2022

@AlCutter
This PR can fix this problem without relying on MySQL configurations and versions, and it is compatible with earlier versions previous to MySQL 8.0.

Fixes #2653

@AlCutter
Copy link
Member

Ah, that's great - thank you!
It might be worth adding a line about adding preliminary support for MySQL 8 into the CHANGELOG file?

@AlCutter
Copy link
Member

/gcbrun

Copy link
Member

@AlCutter AlCutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor nits and a couple of questions on the code itself inline.

return nil
}

return fmt.Errorf("Failed to get variable %q: %v", opt, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "Failed" -> "failed"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

exp, err := strconv.Atoi(expiry)
if err != nil || exp != 0 {
if _, err := db.ExecContext(ctx, "SET SESSION "+opt+"=0"); err != nil {
return fmt.Errorf("Failed to set variable %q: %v", opt, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "Failed" -> "failed"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

opt := "information_schema_stats_expiry"
res := db.QueryRowContext(ctx, "SHOW VARIABLES LIKE '%"+opt+"%'")
var none, expiry string
err := res.Scan(&none, &expiry)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can you inline this with the check on err like this: if err := res.Scan...; err != nil { ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

return fmt.Errorf("Failed to get variable %q: %v", opt, err)
}

exp, err := strconv.Atoi(expiry)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can inline here too?
if exp, err := strconv...; err != nil || exp != 0 { ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

// turnOffInformationSchemaCache turn off statistics caching for MySQL 8
// To always retrieve the latest statistics directly from the storage engine and bypass cached values, set information_schema_stats_expiry to 0.
// See https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_information_schema_stats_expiry
func turnOffInformationSchemaCache(ctx context.Context, db *sql.DB) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this fail safely for all versions of MySQL prior to 8 or should we check the version and only execute on version 8+?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will fail safely for versions before MySQL 8.0. This is because this function uses SHOW VARIABLES LIKE statement. Executing this statement in versions before MySQL 8 is equivalent to searching a system variable that does not exist, and it will only return empty result rather than error.

Furthermore, tests of 5.6.51, 5.7.36 and 8.0.27 have already passed locally.

opt := "information_schema_stats_expiry"
res := db.QueryRowContext(ctx, "SHOW VARIABLES LIKE '%"+opt+"%'")
var none, expiry string
err := res.Scan(&none, &expiry)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to Scan expiry directly into an int or will that fail if it's unset?
(Would changing the query to SELECT FROM @@SESSION.information_schema_stats_expiry help if so?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for your reminding. Indeed, “expiry” should be scanned into int type.

When this variable does not exist (versions before MySQL 8.0), SELECT @@SESSION.information_schema_stats_expiry statement will return error instead of empty resultset.

Standardize error messages and code style.
More rigorous SQL statements.
@px3303 px3303 requested a review from AlCutter January 13, 2022 08:32
@px3303
Copy link
Contributor Author

px3303 commented Jan 13, 2022

@AlCutter

I have updated the CHANGELOG file.

@AlCutter
Copy link
Member

/gcbrun

@AlCutter
Copy link
Member

@px3303 - Nice one, many thanks!

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.

Not compatible with MySQL 8.0
2 participants