ddl: implement `ALTER DATABASE` to alter charset/collation #10393

bb7133 · 2019-05-08T09:12:54Z

What problem does this PR solve?

Add support for ALTER DATABASE syntax

Related issue: #4641
Related parser PR: pingcap/parser#318

What is changed and how it works?

A new DDL job is added to change charset/collate for a database.

Check List

Tests

Integration test

Code changes

Has exported function/method change

Side effects

Increased code complexity

Related changes

Need to cherry-pick to the release branch
Need to update the documentation
Need to be included in the release note

bb7133 · 2019-05-08T09:39:12Z

PTAL @crazycs520 @lonng @winkyao

zhouqiang-cl · 2019-05-08T14:29:27Z

/run-all-tests

codecov · 2019-05-09T13:49:55Z

Codecov Report

Merging #10393 into master will decrease coverage by 0.0281%.
The diff coverage is 71.4285%.

@@               Coverage Diff                @@
##             master     #10393        +/-   ##
================================================
- Coverage   77.3263%   77.2982%   -0.0281%     
================================================
  Files           412        412                
  Lines         86629      86720        +91     
================================================
+ Hits          66987      67033        +46     
- Misses        14498      14528        +30     
- Partials       5144       5159        +15

zhouqiang-cl · 2019-05-10T04:12:55Z

/rebuild

go.sum1

crazycs520

LGTM

crazycs520 · 2019-05-10T12:25:07Z

@bb7133 I got some problem, but maybe we can fix this in other PR:
In this PR

mysql root@localhost:(none)> create database test charset utf8;
Query OK, 0 rows affected
Time: 0.005s
mysql root@localhost:(none)> alter database test charset="utf8"
(1105, u'unsupported modify collate from  to utf8_bin')
mysql root@localhost:(none)> drop database if exists test
Query OK, 0 rows affected
Time: 0.010s
mysql root@localhost:(none)> create database test charset UTF8MB4;
Query OK, 0 rows affected
Time: 0.005s
mysql root@localhost:(none)> alter database test charset="utf8mb4"
(1105, u'unsupported modify collate from  to utf8mb4_bin')
mysql root@localhost:(none)>

There is some bug in create a schema.

bb7133 · 2019-05-11T01:33:20Z

There are some bugs in creating a schema.

tidb/ddl/ddl_api.go

Line 2170 in d241d74

    
           func modifiableCharsetAndCollation(toCharset, toCollate, origCharset, origCollate string) error {

Isn't this an expected behavior? For now, we only support changing charset from utf8 to utf8mb4

bb7133 · 2019-05-11T01:59:10Z

@jackysp @crazycs520 addressed, PTAL, thank you!

winkyao

rest LGTM

winkyao · 2019-05-13T11:40:45Z

ddl/schema.go

+
+	dbInfo, err := checkSchemaExistAndCancelNotExistJob(t, job)
+	if err != nil {
+		if infoschema.ErrDatabaseDropExists.Equal(err) {


This error is weird, I think to call checkSchemaExistAndCancelNotExistJob is not appropriate. Maybe you should call t.GetDatabase(job.SchemaID) directly.

jackysp

LGTM

zimulala · 2019-05-14T06:15:58Z

infoschema/builder.go

+	if !ok {
+		return ErrDatabaseNotExists.GenWithStackByArgs(di.Name.O)
+	}
+	if oldInfo.Charset == di.Charset && oldInfo.Collate == di.Collate {


If this happens, it is already processed when the DDL job is processed. Doesn't this happen here?

I suppose another DDL job may have changed the charset already, is it possible?

I don't think so. If another DDL job changed, this DDL job needn't to be handled.

zimulala · 2019-05-14T06:17:21Z

infoschema/builder.go

+		return errors.Trace(err)
+	}
+	if di == nil {
+		// When we apply an old schema diff, the database may has been dropped already


Ditto. Maybe we can update this comment.

zimulala · 2019-05-14T06:22:50Z

ddl/schema.go

+		}
+		return ver, errors.Trace(err)
+	}
+


I think we can check "if dbInfo.Charset == toCharset && dbInfo.Charset == toCollate {return nil}" here。

zimulala · 2019-05-14T06:24:51Z

ddl/ddl_api.go

+		return errors.Trace(err)
+	}
+
+	// Do DDL job


Suggested change

// Do DDL job

// Do the DDL job.

zimulala · 2019-05-14T06:25:38Z

ddl/ddl_api.go

+		return nil
+	}
+
+	// check current TiDB limitations


Suggested change

// check current TiDB limitations

// Check the current TiDB limitations.

zimulala · 2019-05-14T06:26:44Z

ddl/ddl_api.go

@@ -90,6 +90,63 @@ func (d *ddl) CreateSchema(ctx sessionctx.Context, schema model.CIStr, charsetIn
 	err = d.callHookOnChanged(err)
 	return errors.Trace(err)
 }
+func (d *ddl) AlterSchema(ctx sessionctx.Context, stmt *ast.AlterDatabaseStmt) (err error) {
+	// Resolve target charset and collation from options


Please add the . at the end of a line. And please update other comments in this PR.

bb7133 · 2019-05-14T10:09:37Z

PTAL @winkyao @zimulala , thanks

zimulala · 2019-05-14T10:44:00Z

ddl/schema.go

+	}
+
+	dbInfo, err := t.GetDatabase(job.SchemaID)
+	if err != nil {


Here we need to add job.State = model.JobStateCancelled to make sure the job will be done.

yes, if we don't cancel the job here, the job will never be finished.

zimulala · 2019-05-15T06:23:24Z

ddl/schema.go

+	}
+
+	if dbInfo.Charset == toCharset && dbInfo.Collate == toCollate {
+		job.FinishDBJob(model.JobStateDone, model.StatePublic, ver, dbInfo)


I think replace this line with job.State = model.JobStateCancelled is better.
We don't do this schema change, becase other job is done.

But this will cause an error message, right?

No. Here we return error is nil.

addressed, thanks @zimulala

zimulala

LGTM

zimulala · 2019-05-15T06:48:28Z

/run-all-tests

winkyao

LGTM

bb7133 · 2019-05-15T09:26:51Z

/run-all-tests

…0393)

…10503)

bb7133 added type/enhancement The issue or PR belongs to an enhancement. component/DDL-need-LGT3 labels May 8, 2019

bb7133 force-pushed the bb7133/alter_db_charset branch from e4cd9d0 to f31cc2a Compare May 9, 2019 13:49

jackysp reviewed May 10, 2019

View reviewed changes

go.sum1 Outdated Show resolved Hide resolved

crazycs520 reviewed May 10, 2019

View reviewed changes

bb7133 force-pushed the bb7133/alter_db_charset branch from 16ef132 to 82966c6 Compare May 11, 2019 01:35

winkyao reviewed May 13, 2019

View reviewed changes

jackysp reviewed May 13, 2019

View reviewed changes

zimulala reviewed May 14, 2019

View reviewed changes

bb7133 force-pushed the bb7133/alter_db_charset branch from 1d8cf8d to b3ec525 Compare May 14, 2019 10:07

zimulala reviewed May 14, 2019

View reviewed changes

bb7133 added 6 commits May 15, 2019 14:05

ddl: implement ALTER DATABASE to alter charset/collation

228f7f1

update go.mod / go.sum

7bc6124

remove useless file

718fb56

address comment

452d2ee

address comment and fix typo

62cb0a1

address comment

2f14d76

winkyao added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label May 15, 2019

bb7133 force-pushed the bb7133/alter_db_charset branch from b3ec525 to 2f14d76 Compare May 15, 2019 06:05

zimulala reviewed May 15, 2019

View reviewed changes

address comment

357851a

zimulala approved these changes May 15, 2019

View reviewed changes

winkyao reviewed May 15, 2019

View reviewed changes

winkyao added the status/all tests passed label May 15, 2019

winkyao merged commit 5c21fa4 into pingcap:master May 15, 2019

bb7133 added a commit to bb7133/tidb that referenced this pull request May 16, 2019

ddl: implement ALTER DATABASE to alter charset/collation (pingcap#1…

2a955ab

…0393)

bb7133 mentioned this pull request May 16, 2019

ddl: implement ALTER DATABASE to alter charset/collation (#10393) #10503

Merged

jackysp pushed a commit that referenced this pull request May 20, 2019

ddl: implement ALTER DATABASE to alter charset/collation (#10393) (#…

c612ce9

…10503)

winkyao mentioned this pull request Jul 23, 2019

alter database syntax support. #4641

Closed

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

bb7133 deleted the bb7133/alter_db_charset branch December 29, 2023 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: implement `ALTER DATABASE` to alter charset/collation #10393

ddl: implement `ALTER DATABASE` to alter charset/collation #10393

bb7133 commented May 8, 2019

bb7133 commented May 8, 2019

zhouqiang-cl commented May 8, 2019

codecov bot commented May 9, 2019 •

edited

Loading

zhouqiang-cl commented May 10, 2019

crazycs520 left a comment

crazycs520 commented May 10, 2019

bb7133 commented May 11, 2019

bb7133 commented May 11, 2019

winkyao left a comment

winkyao May 13, 2019

bb7133 May 14, 2019

jackysp left a comment

zimulala May 14, 2019

bb7133 May 14, 2019

zimulala May 14, 2019

zimulala May 14, 2019

zimulala May 14, 2019

bb7133 May 14, 2019

zimulala May 14, 2019

zimulala May 14, 2019

zimulala May 14, 2019

bb7133 commented May 14, 2019

zimulala May 14, 2019

winkyao May 14, 2019

zimulala May 15, 2019

bb7133 May 15, 2019

zimulala May 15, 2019 •

edited

Loading

bb7133 May 15, 2019

zimulala left a comment

zimulala commented May 15, 2019

winkyao left a comment

bb7133 commented May 15, 2019

	// check current TiDB limitations
	// Check the current TiDB limitations.

ddl: implement ALTER DATABASE to alter charset/collation #10393

ddl: implement ALTER DATABASE to alter charset/collation #10393

Conversation

bb7133 commented May 8, 2019

What problem does this PR solve?

What is changed and how it works?

Check List

bb7133 commented May 8, 2019

zhouqiang-cl commented May 8, 2019

codecov bot commented May 9, 2019 • edited Loading

Codecov Report

zhouqiang-cl commented May 10, 2019

crazycs520 left a comment

Choose a reason for hiding this comment

crazycs520 commented May 10, 2019

bb7133 commented May 11, 2019

bb7133 commented May 11, 2019

winkyao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackysp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bb7133 commented May 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala May 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

zimulala commented May 15, 2019

winkyao left a comment

Choose a reason for hiding this comment

bb7133 commented May 15, 2019

ddl: implement `ALTER DATABASE` to alter charset/collation #10393

ddl: implement `ALTER DATABASE` to alter charset/collation #10393

codecov bot commented May 9, 2019 •

edited

Loading

zimulala May 15, 2019 •

edited

Loading