Skip to content

*: create / drop extended stats by ALTER TABLE - #1138

Merged
kennytm merged 2 commits into
pingcap:masterfrom
eurekaka:ext_stats
Jan 5, 2021
Merged

kennytm merged 2 commits into
pingcap:masterfrom
eurekaka:ext_stats

Conversation

@eurekaka

@eurekaka eurekaka commented Dec 24, 2020 •

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Change the extended statistics SQL interfaces according to https://docs.google.com/document/d/1rRIQ8ekoKJH8qeB-J15QRaycLys8_19GBnij7q_O7Vg/edit.

What is changed and how it works?

Use ALTER TABLE ... ADD / DROP STATISTICS syntax for create / drop extended statistics. Old syntax in #913 would be removed later once the new syntax support is finished.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

Side effects

  • Breaking backward compatibility: the old syntax is not generally available now, so it should be fine to be removed.

Related changes

N/A

@eurekaka
eurekaka requested review from qw4990 and winoros December 24, 2020 13:16
@eurekaka eurekaka added the type/enhancement New feature or request label Dec 24, 2020
Comment thread parser.y Outdated
sqlSmallResult "SQL_SMALL_RESULT"
ssl "SSL"
starting "STARTING"
statistics "STATISTICS"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This would make statistics a reserved keyword, while in previous syntax implementation, it is just a TiDB keyword. If we don't do this, the newly added rule would have shift/reduce conflict with ColumnKeywordOpt, because statistics can be treated as a column name for ADD [COLUMN] statistics.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Making statistics or stats a reserved keyword just to avoid having the "frankenstein" CREATE STATISTICS syntax (already used by T-SQL and PostgreSQL) is not a good trade-off imo.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@zz-jason @morgo what's your opinion?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For better syntax compatibility with MySQL, I think it's worthy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and what about TIDB_STATS / TIDB_STATISTICS 🤔

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with kennytm. stats / statistics is not a reserved word in the SQL standard, or in MySQL. It doesn't hurt usability too much to use TIDB_STATISTICS or OPTIMIZER_STATISTICS instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alright, I replaced statistics with tidb_stats, shall we use admin reload statistics or admin reload tidb_stats then?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the consistency of admin reload tidb_stats, but not a strong opinion.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we choose tidb_stats, lots of SQL grammar might need to be modified to keep consistency.

For example, some SQL syntaxes:

show stats_meta;
load stats;

Config:

stats-lease = "3s"

HTTP API:

http://$host:$port/stats/dump/$db/$table

And some tables:

stats_buckets
stats_feedback
stats_histograms
stats_meta
stats_top_n

keeping the name to be consistent helps users to easily remember these concepts. I prefer to use stats for drop and alter table statements.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about replacing TIDB_STATS with STATS_EXTENDED to keep consistency? @zz-jason

ALTER TABLE t ADD stats_extended s1 dependency(a,b);
ALTER TABLE t DROP stats_extended s1;
ADMIN RELOAD stats_extended;

or maybe a shorter STATS_EXT?

@kennytm kennytm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 LGT1 label Jan 2, 2021

@winoros winoros left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@ti-srebot ti-srebot removed the status/LGT1 LGT1 label Jan 5, 2021
@ti-srebot ti-srebot added the status/LGT2 LGT2 label Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/LGT2 LGT2 type/enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants