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

Jump to bottom

table: should not skip when default value is not null #20491

Merged
merged 4 commits into from
Oct 20, 2020
Merged

table: should not skip when default value is not null #20491

merged 4 commits into from
Oct 20, 2020

Conversation

elvizlai
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #20490

Problem Summary:

When new added column changed from not null to null, the default value may not correct.

Release note

@ti-srebot ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Oct 16, 2020
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Oct 16, 2020
@elvizlai
Copy link
Contributor Author

elvizlai commented Oct 17, 2020 •
edited
Loading

the cause of this may from L273

tidb/server/util.go

Lines 270 to 276 in 62190f3

func dumpTextRow(buffer []byte, columns []*ColumnInfo, row chunk.Row) ([]byte, error) {
tmp := make([]byte, 0, 20)
for i, col := range columns {
if row.IsNull(i) {
buffer = append(buffer, 0xfb)
continue
}

when the new added column with not null modify to null, the column related meta info may not changed correctly, that cause row.IsNull(idx) be false.

@bb7133
Copy link
Member

bb7133 commented Oct 19, 2020

Hi @elvizlai , thanks for your contribution! Would you please add some test cases?

@bb7133
Copy link
Member

bb7133 commented Oct 19, 2020

Unfortunately, we've some failed test cases(https://internal.pingcap.net/idc-jenkins/blue/organizations/jenkins/tidb_ghpr_check_2/detail/tidb_ghpr_check_2/55626/pipeline/):

[2020-10-16T08:12:26.095Z] FAIL: pessimistic_test.go:1573: testPessimisticSuite.TestPessimisticTxnWithDDLChangeColumn
[2020-10-16T08:12:26.095Z] 
[2020-10-16T08:12:26.095Z] pessimistic_test.go:1636:
[2020-10-16T08:12:26.095Z]     tk2.MustQuery("select count(1) from tbl_time where c_time is not null").Check(testkit.Rows("2"))
[2020-10-16T08:12:26.096Z] /home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:63:
[2020-10-16T08:12:26.096Z]     res.c.Assert(resBuff.String(), check.Equals, needBuff.String(), res.comment)
[2020-10-16T08:12:26.096Z] ... obtained string = "[3]\n"
[2020-10-16T08:12:26.096Z] ... expected string = "[2]\n"
[2020-10-16T08:12:26.096Z] ... sql:select count(1) from tbl_time where c_time is not null, args:[]

The case can be simply reproduced by:

tidb> create table tbl_time(c1 int, c_time timestamp);                                                                                  Query OK, 0 rows affected (0.00 sec)

tidb> insert into tbl_time(c1) values(1);
Query OK, 1 row affected (0.00 sec)

tidb> select * from tbl_time;
+------+--------+
| c1   | c_time |
+------+--------+
|    1 | NULL   |
+------+--------+
1 row in set (0.00 sec)

tidb> alter table tbl_time modify c_time timestamp default now();                                                                       Query OK, 0 rows affected (0.01 sec)

tidb> insert into tbl_time(c1) values(2);
Query OK, 1 row affected (0.00 sec)

tidb> insert into tbl_time(c1) values(3);
Query OK, 1 row affected (0.00 sec)

tidb> select * from tbl_time;
+------+---------------------+
| c1   | c_time              |
+------+---------------------+
|    1 | 2020-10-19 09:44:17 |
|    2 | 2020-10-19 09:44:32 |
|    3 | 2020-10-19 09:44:34 |
+------+---------------------+
3 rows in set (0.00 sec)

Please notice that the c_time value for c1=1 record is filled unexpectedly.

@elvizlai elvizlai changed the title ddl: should regenerate default value when modify column [WIP]ddl: should regenerate default value when modify column Oct 19, 2020
@elvizlai elvizlai changed the title [WIP]ddl: should regenerate default value when modify column [WIP]table: should not skip when default value is not null Oct 19, 2020
@elvizlai elvizlai changed the title [WIP]table: should not skip when default value is not null table: should not skip when default value is not null Oct 19, 2020
@elvizlai
Copy link
Contributor Author

elvizlai commented Oct 19, 2020 •
edited
Loading

PTAL

my local test was passed.

➜  tidb git:(fix_20490) make test
cat checklist.md
# Following the checklist saves the reviewers' time and gets your PR reviewed faster.

# Self Review
Have you reviewed every line of your changes by yourself?

# Test
Have you added enough test cases to cover the new feature or bug fix?
Also, add comments to describe your test cases.

# Naming
Do function names keep consistent with its behavior?
Is it easy to infer the function's behavior by its name?

# Comment
Is there any code that confuses the reviewer?
Add comments on them! You'll be asked to do so anyway.
Make sure there is no syntax or spelling error in your comments.
Some online syntax checking tools like Grammarly may be helpful.

# Refactor
Is there any way to refactor the code to make it more readable?
If the refactoring touches a lot of existing code, send another PR to do it.

# Single Purpose
Make sure the PR does only one thing and nothing else.

# Diff Size
Make sure the diff size is no more than 500, split it into small PRs if it is too large.
GO111MODULE=on go build  -tags codes  -ldflags '-X "github.com/pingcap/parser/mysql.TiDBReleaseVersion=v4.0.0-beta.2-1395-g0f8aa882d" -X "github.com/pingcap/tidb/util/versioninfo.TiDBBuildTS=2020-10-19 14:12:54" -X "github.com/pingcap/tidb/util/versioninfo.TiDBGitHash=0f8aa882d95365ebc2d182abdadc8120caab5cf9" -X "github.com/pingcap/tidb/util/versioninfo.TiDBGitBranch=fix_20490" -X "github.com/pingcap/tidb/util/versioninfo.TiDBEdition=Community" -X "github.com/pingcap/tidb/config.checkBeforeDropLDFlag=1"' -o bin/tidb-server tidb-server/main.go
skip building tidb-server, using existing binary: ../../bin/tidb-server
building portgenerator binary: ./portgenerator
building explain-test binary: ./explain_test
start tidb-server, log file: ./explain-test.out
tidb-server(PID: 7707) started
run all explain test cases
explaintest end
GO111MODULE=on go list -f '{{ join .Imports "\n" }}' github.com/pingcap/tidb/store/tikv | grep ^github.com/pingcap/parser$ || exit 0; exit 1
GO111MODULE=on go build -o tools/bin/failpoint-ctl github.com/pingcap/failpoint/failpoint-ctl
go: downloading github.com/sergi/go-diff v1.0.1-0.20180205163309-da645544ed44
Running in native mode.
ok  	github.com/pingcap/tidb/bindinfo	2.526s	coverage: 85.8% of statements
ok  	github.com/pingcap/tidb/config	0.299s	coverage: 66.7% of statements
ok  	github.com/pingcap/tidb/ddl	66.461s	coverage: 85.3% of statements
ok  	github.com/pingcap/tidb/ddl/failtest	5.845s	coverage: [no statements]
ok  	github.com/pingcap/tidb/ddl/placement	0.017s	coverage: 22.6% of statements
?   	github.com/pingcap/tidb/ddl/testutil	[no test files]
ok  	github.com/pingcap/tidb/ddl/util	1.813s	coverage: 46.0% of statements
ok  	github.com/pingcap/tidb/distsql	0.063s	coverage: 71.0% of statements
ok  	github.com/pingcap/tidb/domain	2.794s	coverage: 81.9% of statements
ok  	github.com/pingcap/tidb/domain/infosync	1.138s	coverage: 16.3% of statements
?   	github.com/pingcap/tidb/errno	[no test files]
ok  	github.com/pingcap/tidb/executor	61.742s	coverage: 77.2% of statements
ok  	github.com/pingcap/tidb/executor/aggfuncs	1.542s	coverage: 75.9% of statements
ok  	github.com/pingcap/tidb/executor/oomtest	0.048s	coverage: [no statements] [no tests to run]
ok  	github.com/pingcap/tidb/executor/seqtest	5.165s	coverage: [no statements]
ok  	github.com/pingcap/tidb/expression	12.322s	coverage: 82.2% of statements
ok  	github.com/pingcap/tidb/expression/aggregation	0.049s	coverage: 44.5% of statements
?   	github.com/pingcap/tidb/expression/generator/helper	[no test files]
ok  	github.com/pingcap/tidb/infoschema	4.036s	coverage: 57.5% of statements
ok  	github.com/pingcap/tidb/infoschema/perfschema	0.623s	coverage: 70.3% of statements
ok  	github.com/pingcap/tidb/kv	60.290s	coverage: 78.8% of statements
?   	github.com/pingcap/tidb/lock	[no test files]
ok  	github.com/pingcap/tidb/meta	1.140s	coverage: 74.1% of statements
ok  	github.com/pingcap/tidb/meta/autoid	1.486s	coverage: 73.2% of statements
ok  	github.com/pingcap/tidb/metrics	0.019s	coverage: 100.0% of statements
ok  	github.com/pingcap/tidb/owner	20.824s	coverage: 69.6% of statements
?   	github.com/pingcap/tidb/planner	[no test files]
ok  	github.com/pingcap/tidb/planner/cascades	0.898s	coverage: 94.5% of statements
ok  	github.com/pingcap/tidb/planner/core	34.884s	coverage: 78.8% of statements
ok  	github.com/pingcap/tidb/planner/implementation	0.028s	coverage: 3.7% of statements
ok  	github.com/pingcap/tidb/planner/memo	0.029s	coverage: 86.9% of statements
?   	github.com/pingcap/tidb/planner/property	[no test files]
?   	github.com/pingcap/tidb/planner/util	[no test files]
ok  	github.com/pingcap/tidb/plugin	0.029s	coverage: 59.0% of statements
ok  	github.com/pingcap/tidb/plugin/conn_ip_example	0.025s	coverage: 0.0% of statements
?   	github.com/pingcap/tidb/privilege	[no test files]
ok  	github.com/pingcap/tidb/privilege/privileges	2.512s	coverage: 82.1% of statements
ok  	github.com/pingcap/tidb/server	43.992s	coverage: 67.0% of statements
ok  	github.com/pingcap/tidb/session	28.477s	coverage: 75.0% of statements
ok  	github.com/pingcap/tidb/sessionctx	0.019s	coverage: 83.3% of statements
ok  	github.com/pingcap/tidb/sessionctx/binloginfo	1.642s	coverage: 73.4% of statements
ok  	github.com/pingcap/tidb/sessionctx/stmtctx	0.014s	coverage: 32.1% of statements
ok  	github.com/pingcap/tidb/sessionctx/variable	0.027s	coverage: 50.5% of statements
ok  	github.com/pingcap/tidb/statistics	5.811s	coverage: 76.8% of statements
ok  	github.com/pingcap/tidb/statistics/handle	27.669s	coverage: 85.6% of statements
ok  	github.com/pingcap/tidb/store	4.217s	coverage: 95.5% of statements
ok  	github.com/pingcap/tidb/store/helper	0.299s	coverage: 40.6% of statements
?   	github.com/pingcap/tidb/store/mockoracle	[no test files]
ok  	github.com/pingcap/tidb/store/mockstore	0.029s	coverage: 45.2% of statements
?   	github.com/pingcap/tidb/store/mockstore/cluster	[no test files]
ok  	github.com/pingcap/tidb/store/mockstore/mocktikv	0.357s	coverage: 44.3% of statements
ok  	github.com/pingcap/tidb/store/mockstore/unistore	0.025s	coverage: 20.0% of statements
ok  	github.com/pingcap/tidb/store/mockstore/unistore/cophandler	0.229s	coverage: 21.3% of statements

ok  	github.com/pingcap/tidb/store/tikv	160.477s	coverage: 71.2% of statements
ok  	github.com/pingcap/tidb/store/tikv/gcworker	18.005s	coverage: 76.7% of statements
ok  	github.com/pingcap/tidb/store/tikv/latch	0.018s	coverage: 95.2% of statements
?   	github.com/pingcap/tidb/store/tikv/oracle	[no test files]
ok  	github.com/pingcap/tidb/store/tikv/oracle/oracles	0.377s	coverage: 30.0% of statements
ok  	github.com/pingcap/tidb/store/tikv/tikvrpc	0.021s	coverage: 0.6% of statements
ok  	github.com/pingcap/tidb/structure	0.226s	coverage: 81.2% of statements
ok  	github.com/pingcap/tidb/table	0.029s	coverage: 77.5% of statements
ok  	github.com/pingcap/tidb/table/tables	1.474s	coverage: 64.2% of statements
ok  	github.com/pingcap/tidb/tablecodec	0.023s	coverage: 40.5% of statements
ok  	github.com/pingcap/tidb/telemetry	2.618s	coverage: 57.7% of statements
ok  	github.com/pingcap/tidb/tidb-server	0.039s	coverage: 20.8% of statements
ok  	github.com/pingcap/tidb/types	0.590s	coverage: 82.9% of statements
ok  	github.com/pingcap/tidb/types/json	0.019s	coverage: 82.5% of statements
ok  	github.com/pingcap/tidb/types/parser_driver	0.026s	coverage: 28.4% of statements
ok  	github.com/pingcap/tidb/util	0.301s	coverage: 52.8% of statements
ok  	github.com/pingcap/tidb/util/admin	1.344s	coverage: 53.5% of statements
ok  	github.com/pingcap/tidb/util/arena	0.011s	coverage: 100.0% of statements
ok  	github.com/pingcap/tidb/util/bitmap	0.200s	coverage: 85.7% of statements
ok  	github.com/pingcap/tidb/util/checksum	0.025s	coverage: 85.1% of statements
ok  	github.com/pingcap/tidb/util/chunk	2.620s	coverage: 79.2% of statements
ok  	github.com/pingcap/tidb/util/codec	0.029s	coverage: 82.6% of statements
ok  	github.com/pingcap/tidb/util/collate	0.016s	coverage: 47.4% of statements
ok  	github.com/pingcap/tidb/util/deadlock	0.014s	coverage: 100.0% of statements
ok  	github.com/pingcap/tidb/util/disjointset	0.011s	coverage: 100.0% of statements
ok  	github.com/pingcap/tidb/util/disk	0.018s	coverage: 60.5% of statements
?   	github.com/pingcap/tidb/util/domainutil	[no test files]
ok  	github.com/pingcap/tidb/util/encrypt	0.018s	coverage: 92.6% of statements
ok  	github.com/pingcap/tidb/util/execdetails	0.014s	coverage: 63.4% of statements
ok  	github.com/pingcap/tidb/util/expensivequery	0.022s	coverage: 13.5% of statements
ok  	github.com/pingcap/tidb/util/fastrand	0.011s	coverage: 0.0% of statements [no tests to run]
ok  	github.com/pingcap/tidb/util/filesort	0.550s	coverage: 86.3% of statements
ok  	github.com/pingcap/tidb/util/format	0.013s	coverage: 83.6% of statements
?   	github.com/pingcap/tidb/util/gcutil	[no test files]
ok  	github.com/pingcap/tidb/util/generatedexpr	0.020s	coverage: 0.0% of statements [no tests to run]
ok  	github.com/pingcap/tidb/util/hack	0.016s	coverage: 92.3% of statements
?   	github.com/pingcap/tidb/util/hint	[no test files]
?   	github.com/pingcap/tidb/util/israce	[no test files]
ok  	github.com/pingcap/tidb/util/kvcache	0.016s	coverage: 53.6% of statements
ok  	github.com/pingcap/tidb/util/localpool	0.017s	coverage: 96.8% of statements
ok  	github.com/pingcap/tidb/util/logutil	0.022s	coverage: 77.6% of statements
ok  	github.com/pingcap/tidb/util/math	0.489s	coverage: 33.3% of statements
ok  	github.com/pingcap/tidb/util/memory	0.018s	coverage: 72.6% of statements
ok  	github.com/pingcap/tidb/util/mock	0.021s	coverage: 19.0% of statements
ok  	github.com/pingcap/tidb/util/mvmap	0.012s	coverage: 86.6% of statements
ok  	github.com/pingcap/tidb/util/parser	0.014s	coverage: 56.4% of statements
?   	github.com/pingcap/tidb/util/pdapi	[no test files]
ok  	github.com/pingcap/tidb/util/plancodec	0.028s	coverage: 4.2% of statements
ok  	github.com/pingcap/tidb/util/printer	0.017s	coverage: 88.3% of statements
ok  	github.com/pingcap/tidb/util/profile	0.727s	coverage: 55.4% of statements
ok  	github.com/pingcap/tidb/util/ranger	2.025s	coverage: 80.3% of statements
?   	github.com/pingcap/tidb/util/redact	[no test files]
ok  	github.com/pingcap/tidb/util/rowDecoder	0.032s	coverage: 81.0% of statements
ok  	github.com/pingcap/tidb/util/rowcodec	0.046s	coverage: 88.1% of statements
ok  	github.com/pingcap/tidb/util/selection	1.546s	coverage: 85.7% of statements
ok  	github.com/pingcap/tidb/util/set	0.012s	coverage: 91.9% of statements
?   	github.com/pingcap/tidb/util/signal	[no test files]
?   	github.com/pingcap/tidb/util/sqlexec	[no test files]
ok  	github.com/pingcap/tidb/util/stmtsummary	0.037s	coverage: 91.5% of statements
?   	github.com/pingcap/tidb/util/storeutil	[no test files]
ok  	github.com/pingcap/tidb/util/stringutil	0.018s	coverage: 92.5% of statements
ok  	github.com/pingcap/tidb/util/sys/linux	0.017s	coverage: 66.7% of statements
ok  	github.com/pingcap/tidb/util/sys/storage	0.012s	coverage: 83.3% of statements
ok  	github.com/pingcap/tidb/util/systimemon	1.012s	coverage: 85.7% of statements
ok  	github.com/pingcap/tidb/util/testkit	0.033s	coverage: 6.5% of statements
?   	github.com/pingcap/tidb/util/testleak	[no test files]
ok  	github.com/pingcap/tidb/util/testutil	0.019s	coverage: 18.7% of statements
ok  	github.com/pingcap/tidb/util/texttree	0.011s	coverage: 100.0% of statements
ok  	github.com/pingcap/tidb/util/timeutil	0.012s	coverage: 49.2% of statements
ok  	github.com/pingcap/tidb/util/tracing	0.013s	coverage: 93.3% of statements
?   	github.com/pingcap/tidb/util/versioninfo	[no test files]
go generate ./...
./tools/check/check-gogenerate.sh
Great, all tests passed.

@bb7133
Copy link
Member

bb7133 commented Oct 20, 2020

/run-all-tests

@bb7133
Copy link
Member

bb7133 commented Oct 20, 2020

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 20, 2020
@bb7133
Copy link
Member

bb7133 commented Oct 20, 2020

PTAL @tangenta , @djshow832

AilinKid
AilinKid reviewed Oct 20, 2020
View reviewed changes
Copy link
Contributor

@AilinKid AilinKid left a comment

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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 20, 2020
tangenta
tangenta reviewed Oct 20, 2020
View reviewed changes
Copy link
Contributor

@tangenta tangenta left a comment

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 status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Oct 20, 2020
@bb7133
Copy link
Member

bb7133 commented Oct 20, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 20, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 0806920 into pingcap:master Oct 20, 2020
@ti-srebot ti-srebot mentioned this pull request Oct 20, 2020
Merged
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Oct 20, 2020
@elvizlai @ti-srebot
cherry pick pingcap#20491 to release-3.0
10ddf37 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #20532

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Oct 20, 2020
@elvizlai @ti-srebot
cherry pick pingcap#20491 to release-4.0
fd4b9ca 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Oct 20, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #20533

bb7133 pushed a commit that referenced this pull request Oct 28, 2020
@ti-srebot
table: should not skip when default value is not null (#20491) (#20532)
87a2cf7
bb7133 pushed a commit that referenced this pull request Nov 11, 2020
@ti-srebot
table: should not skip when default value is not null (#20491) (#20533)
737fe82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AilinKid AilinKid AilinKid left review comments

@tangenta tangenta tangenta left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees
No one assigned
Labels
first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ddl: new added not null column with default changed to null result behavior not expected
5 participants
@elvizlai @bb7133 @ti-srebot @AilinKid @tangenta