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

tpch: add query tuning configs for tpch #179

Merged
merged 7 commits into from
Aug 20, 2024

Conversation

zanmato1984
Copy link
Contributor

@zanmato1984 zanmato1984 commented Aug 14, 2024

Set some session variables known to be effective for tpch before each tpch query to tune performance. And also add flags to turn it on/off and override the default variables.

      --enable-query-tuning         Enable query tuning by setting specified session variables (default true)

tidb_default_string_match_selectivity=0.1: For optimal join order, esp. q9.
tidb_opt_join_reorder_threshold=60: For general optimal join order.
tidb_prefer_broadcast_join_by_exchange_data_size=ON: For better join type between broadcast join and partition join.

@CLAassistant
Copy link

CLAassistant commented Aug 14, 2024

CLA assistant check
All committers have signed the CLA.

@Yui-Song
Copy link
Collaborator

  • Go-tpc already has a global flag to set session variables. It would be better if you could leverage it to set those variables.
--conn-params string    session variables, e.g. for TiDB --conn-params tidb_isolation_read_engines='tiflash', For PostgreSQL: --conn-params sslmode=disable
  • And how about checking whether the user is using TiDB and then deciding to set those session variables or not? The user may get errors if they are testing other MySQL-compatible databases when go-tpc tries to set those tidb dedicated variables.

@Yui-Song Yui-Song assigned Yui-Song and unassigned Yui-Song Aug 14, 2024
@Yui-Song Yui-Song self-requested a review August 14, 2024 11:04
@zanmato1984
Copy link
Contributor Author

zanmato1984 commented Aug 14, 2024

  • Go-tpc already has a global flag to set session variables. It would be better if you could leverage it to set those variables.
--conn-params string    session variables, e.g. for TiDB --conn-params tidb_isolation_read_engines='tiflash', For PostgreSQL: --conn-params sslmode=disable

I didn't realize that there exists such a flag. And yes, we should definitely do so. Thanks for pointing this out. I will update soon.

  • And how about checking whether the user is using TiDB and then deciding to set those session variables or not? The user may get errors if they are testing other MySQL-compatible databases when go-tpc tries to set those tidb dedicated variables.

Is there an existing handy utility that I can use to differentiate tidb from other mysql-compatible databases?

EDIT: And also I found there are already tidb-specific variables being used in, e.g.:

go-tpc/tpch/workload.go

Lines 175 to 180 in 50e155e

if w.cfg.Driver == "mysql" {
for _, tbl := range allTables {
fmt.Printf("analyzing table %s\n", tbl)
if _, err := s.Conn.ExecContext(ctx, fmt.Sprintf("SET @@session.tidb_build_stats_concurrency=%d; SET @@session.tidb_distsql_scan_concurrency=%d; SET @@session.tidb_index_serial_scan_concurrency=%d; ANALYZE TABLE %s", acfg.BuildStatsConcurrency, acfg.DistsqlScanConcurrency, acfg.IndexSerialScanConcurrency, tbl)); err != nil {
return err
}

Possibly meaning that current "mysql" can only be tidb?

func executeTpch(action string) {
if action == "run" && driver == "mysql" && tpchConfig.EnableQueryTuning {
Copy link
Contributor Author

@zanmato1984 zanmato1984 Aug 19, 2024

Choose a reason for hiding this comment

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

This has to come prior to openDB() call because it leverages connection parameters which only take effect when connecting to the database.

@zanmato1984
Copy link
Contributor Author

  • Go-tpc already has a global flag to set session variables. It would be better if you could leverage it to set those variables.
--conn-params string    session variables, e.g. for TiDB --conn-params tidb_isolation_read_engines='tiflash', For PostgreSQL: --conn-params sslmode=disable
  • And how about checking whether the user is using TiDB and then deciding to set those session variables or not? The user may get errors if they are testing other MySQL-compatible databases when go-tpc tries to set those tidb dedicated variables.

I've updated the PR by leveraging existing connection parameters rather than explicitly setting session variables via SQL.

About your second concern, the best I can do so far is to check for "mysql" only because there is no existing way to differentiate tidb from other possible mysql vendors, which is also affecting other tidb-specific logic.

@Yui-Song Could you please take a look? Thanks.

@zanmato1984 zanmato1984 merged commit ccd5fcd into pingcap:master Aug 20, 2024
2 checks passed
@zanmato1984 zanmato1984 deleted the tpch-tune branch August 20, 2024 09:30
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.

5 participants