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.

Sign up for GitHub

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

session: skip creating indexes on the analyze_jobs table for older clusters #58608

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+8 −46
Open

session: skip creating indexes on the analyze_jobs table for older clusters #58608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd5f95c
session: skip creating indexes on the analyze_jobs table for older cl…
Rustin170506 Dec 30, 2024
6e70b5b
test: simplify the cases
Rustin170506 Dec 30, 2024
7a52e88
test: update the broken test case
Rustin170506 Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion br/pkg/restore/snap_client/systable_restore_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,5 +116,5 @@ func TestCheckSysTableCompatibility(t *testing.T) {
//
// The above variables are in the file br/pkg/restore/systable_restore.go
func TestMonitorTheSystemTableIncremental(t *testing.T) {
require.Equal(t, int64(241), session.CurrentBootstrapVersion)
require.Equal(t, int64(240), session.CurrentBootstrapVersion)
}
27 changes: 2 additions & 25 deletions pkg/session/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1236,17 +1236,13 @@ const (
// add modify_params to tidb_global_task and tidb_global_task_history.
version239 = 239

// version 240
// Add indexes to mysql.analyze_jobs to speed up the query.
version240 = 240

// Add index on user field for some mysql tables.
version241 = 241
version240 = 240
)

// currentBootstrapVersion is defined as a variable, so we can modify its value for testing.
// please make sure this is the largest version
var currentBootstrapVersion int64 = version241
var currentBootstrapVersion int64 = version240

// DDL owner key's expired time is ManagerSessionTTL seconds, we should wait the time and give more time to have a chance to finish it.
var internalSQLTimeout = owner.ManagerSessionTTL + 15
Expand Down Expand Up @@ -1422,7 +1418,6 @@ var (
upgradeToVer218,
upgradeToVer239,
upgradeToVer240,
upgradeToVer241,
}
)

Expand Down Expand Up @@ -3334,28 +3329,10 @@ func upgradeToVer239(s sessiontypes.Session, ver int64) {
doReentrantDDL(s, "ALTER TABLE mysql.tidb_global_task_history ADD COLUMN modify_params json AFTER `error`;", infoschema.ErrColumnExists)
}

const (
// addAnalyzeJobsSchemaTableStateIndex is a DDL statement that adds an index on (table_schema, table_name, state)
// columns to mysql.analyze_jobs table. This index is currently unused since queries filter on partition_name='',
// even for non-partitioned tables. It is kept for potential future optimization where queries could use this
// simpler index directly for non-partitioned tables.
addAnalyzeJobsSchemaTableStateIndex = "ALTER TABLE mysql.analyze_jobs ADD INDEX idx_schema_table_state (table_schema, table_name, state)"
// addAnalyzeJobsSchemaTablePartitionStateIndex adds an index on (table_schema, table_name, partition_name, state) to mysql.analyze_jobs
addAnalyzeJobsSchemaTablePartitionStateIndex = "ALTER TABLE mysql.analyze_jobs ADD INDEX idx_schema_table_partition_state (table_schema, table_name, partition_name, state)"
)

func upgradeToVer240(s sessiontypes.Session, ver int64) {
if ver >= version240 {
return
}
doReentrantDDL(s, addAnalyzeJobsSchemaTableStateIndex, dbterror.ErrDupKeyName)
doReentrantDDL(s, addAnalyzeJobsSchemaTablePartitionStateIndex, dbterror.ErrDupKeyName)
Comment on lines -3351 to -3352
Copy link
Contributor

@D3Hunter D3Hunter Dec 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

personally, i don't like the idea that for two clusters of same version, but with different schema. It introduce more maintenance burden, if I don't know this PR, and meet a issue and see this difference, I will take it as a bug at first glance.

for a production cluster with so many tables, say 1M, the upgrade duration is quite long in most cases, not just the process of TiDB upgrade, also for rolling upgrade of other components, it's even longer when there are many online traffic, takes hours or even days. I think it's acceptable for this add-index to be slower, and it's the tradeoff we have to make

If we can reduce the size of this table, or the index create faster, that would be better, certainly.

Copy link
Member Author

Choose a reason for hiding this comment

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

personally, i don't like the idea that for two clusters of same version, but with different schema. It introduce more maintenance burden, if I don't know this PR, and meet a issue and see this difference, I will take it as a bug at first glance.

Yes. I agree that having different schemas is annoying. But for most users the full table scan is OK. So introducing the prototail risk to slow down the upgrade is not worth it. We don't want to introduce that risk for users that 99% of them don't have the problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also asked @tangenta, he suggested that it is better not to do this kind of operation for a volumetric table.

Copy link
Contributor

Choose a reason for hiding this comment

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

99% of them don't have the problem.

99% users don't have 1M tables, so upgrade is fast even with the index. this PR is for the 1%

Copy link
Member Author

Choose a reason for hiding this comment

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

/hold

I am not in a hurry with this PR. So let's discuss it further.

Copy link
Member Author

Choose a reason for hiding this comment

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

99% users don't have 1M tables, so upgrade is fast even with the index. this PR is for the 1%

My point here is that 99% of users do not have 1M tables, so there is no need to add this index for them to bring the potential risk for them.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMU, 99% of users do not have 1M tables -> 99% users don't have 100K rows -> add-index very fast -> no such risk

Copy link
Member

Choose a reason for hiding this comment

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

We may also need to add new indexes on tables like mysql.stats_histograms.
This table is related to column count and index count. It's more likely to be a big table.
So the problem will still exist.

I think we can add the related operation to our upgrading guide.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can add the related operation to our upgrading guide.

you mean ask users to manually create index on system table? system table should better be managed by TiDB itself IMO, and most production cluster have very strict permission control, asking DBA or others with root permission to manage what TiDB itself should done, not sure how much they will buy this idea.

}

func upgradeToVer241(s sessiontypes.Session, ver int64) {
if ver >= version241 {
return
}
doReentrantDDL(s, "ALTER TABLE mysql.user ADD INDEX i_user (user)", dbterror.ErrDupKeyName)
doReentrantDDL(s, "ALTER TABLE mysql.global_priv ADD INDEX i_user (user)", dbterror.ErrDupKeyName)
doReentrantDDL(s, "ALTER TABLE mysql.db ADD INDEX i_user (user)", dbterror.ErrDupKeyName)
Expand Down
25 changes: 5 additions & 20 deletions pkg/session/bootstrap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2615,10 +2615,13 @@ func (mebd *mockEtcdBackend) TLSConfig() *tls.Config { return nil }

func (mebd *mockEtcdBackend) StartGCWorker() error { return nil }

func TestTiDBUpgradeToVer240(t *testing.T) {
func TestAnalyzeJobsIndexes(t *testing.T) {
ctx := context.Background()
store, dom := CreateStoreAndBootstrap(t)
defer func() { require.NoError(t, store.Close()) }()
defer func() {
dom.Close()
require.NoError(t, store.Close())
}()

ver239 := version239
seV239 := CreateSessionAndSetID(t, store)
Expand All @@ -2640,24 +2643,6 @@ func TestTiDBUpgradeToVer240(t *testing.T) {
require.Equal(t, 1, chk.NumRows())
require.Contains(t, string(chk.GetRow(0).GetBytes(1)), "idx_schema_table_state")
require.Contains(t, string(chk.GetRow(0).GetBytes(1)), "idx_schema_table_partition_state")

// Check that the indexes still exist after upgrading to the new version and that no errors occurred during the upgrade.
dom.Close()
domCurVer, err := BootstrapSession(store)
require.NoError(t, err)
defer domCurVer.Close()
seCurVer := CreateSessionAndSetID(t, store)
ver, err := getBootstrapVersion(seCurVer)
require.NoError(t, err)
require.Equal(t, currentBootstrapVersion, ver)

res = MustExecToRecodeSet(t, seCurVer, "show create table mysql.analyze_jobs")
chk = res.NewChunk(nil)
err = res.Next(ctx, chk)
require.NoError(t, err)
require.Equal(t, 1, chk.NumRows())
require.Contains(t, string(chk.GetRow(0).GetBytes(1)), "idx_schema_table_state")
require.Contains(t, string(chk.GetRow(0).GetBytes(1)), "idx_schema_table_partition_state")
}

// testExampleAFunc is a example func for TestGetFuncName
Expand Down