Add default value when generate the update rows #1006

july2993 · 2020-11-03T06:01:49Z

What problem does this PR solve?

What is changed and how it works?

only on the condition PreWrite.schemaVersion < schemaVersion of the
table replicated to downstream.

Check List

Tests

Manual test (add detailed scripts or steps below)

enable amend:
set global tidb_enable_amend_pessimistic_txn = 1;

run:

/* INIT */ drop table if exists t;
/* INIT */ create table t (id int primary key, c_str varchar(20));
/* INIT */ insert into t values (1, '0001'), (2, '0002'), (3, null), (4, '0003'), (5, null);
/* TXN */ begin;
/* TXN */ insert into t values (6, '0004');
/* TXN */ insert into t values (7, null);
/* DDL */ alter table t add c_str_new varchar(20);   # IN ANOTHER SESSION
/* TXN */ update t set c_str = '0005' where id = 1;
/* TXN */ update t set c_str = null where id = 2;
/* TXN */ update t set c_str = '0006' where id = 3;
/* TXN */ delete from t where id = 4;
/* TXN */ delete from t where id = 5;
/* TXN */ commit;
/* TXN */ select * from t order by id;
/* TXN */ admin check table t;

Code changes

Side effects

Release note

No Release note.

only on the condition PreWrite.schemaVersion < schemaVersion of the table replicated to downstream.

july2993 · 2020-11-03T06:02:21Z

/run-all-tests tidb=release-4.0 tikv=release-4.0 pd=release-4.0

you06 · 2020-11-03T07:09:12Z

Tested with this PR got unexpected NULL value in down stream

MySQL [test]> select * from accounts;                                                          │MySQL [test]> select * from accounts;
+----+---------+------+------+                                                                 │+----+---------+------+------+
| id | balance | aa   | bb   |                                                                 │| id | balance | aa   | bb   |
+----+---------+------+------+                                                                 │+----+---------+------+------+
|  1 |       1 |    1 |   10 |                                                                 │|  1 |       1 |    1 | NULL |
|  2 |       2 |    2 |   10 |                                                                 │|  2 |       2 |    2 | NULL |
|  3 |       3 |    3 |   10 |                                                                 │|  3 |       3 |    3 | NULL |
+----+---------+------+------+                                                                 │+----+---------+------+------+
3 rows in set (0.063 sec)                                                                      │3 rows in set (0.098 sec)
                                                                                               │
MySQL [test]> show create table accounts;                                                      │MySQL [test]> show create table accounts;
+----------+-----------------------------------------------------------------------------------│+----------+----------------------------------------------------------------------------------
-----------------------------------------------------------------------------------------------│----------------------------------------------------------------------------------------------
------------------------------------------------+                                              │--------------------------------------------------+
| Table    | Create Table                                                                      │| Table    | Create Table                                                                     
                                                                                               │                                                                                              
                                                |                                              │                                                  |
+----------+-----------------------------------------------------------------------------------│+----------+----------------------------------------------------------------------------------
-----------------------------------------------------------------------------------------------│----------------------------------------------------------------------------------------------
------------------------------------------------+                                              │--------------------------------------------------+
| accounts | CREATE TABLE `accounts` (                                                         │| accounts | CREATE TABLE `accounts` (
  `id` int(11) NOT NULL,                                                                       │  `id` int(11) NOT NULL,
  `balance` bigint(20) DEFAULT NULL,                                                           │  `balance` bigint(20) DEFAULT NULL,
  `aa` int(11) DEFAULT NULL,                                                                   │  `aa` int(11) DEFAULT NULL,
  `bb` int(11) DEFAULT 10,                                                                     │  `bb` int(11) DEFAULT 10,
  PRIMARY KEY (`id`)                                                                           │  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |                                  │) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |
+----------+-----------------------------------------------------------------------------------│+----------+----------------------------------------------------------------------------------
-----------------------------------------------------------------------------------------------│----------------------------------------------------------------------------------------------
------------------------------------------------+                                              │--------------------------------------------------+
1 row in set (0.051 sec)                                                                       │1 row in set (0.124 sec)
                                                                                               │

july2993 · 2020-11-03T07:32:33Z

Tested with this PR got unexpected NULL value in down stream

MySQL [test]> select * from accounts;                                                          │MySQL [test]> select * from accounts;
+----+---------+------+------+                                                                 │+----+---------+------+------+
| id | balance | aa   | bb   |                                                                 │| id | balance | aa   | bb   |
+----+---------+------+------+                                                                 │+----+---------+------+------+
|  1 |       1 |    1 |   10 |                                                                 │|  1 |       1 |    1 | NULL |
|  2 |       2 |    2 |   10 |                                                                 │|  2 |       2 |    2 | NULL |
|  3 |       3 |    3 |   10 |                                                                 │|  3 |       3 |    3 | NULL |
+----+---------+------+------+                                                                 │+----+---------+------+------+
3 rows in set (0.063 sec)                                                                      │3 rows in set (0.098 sec)
                                                                                               │
MySQL [test]> show create table accounts;                                                      │MySQL [test]> show create table accounts;
+----------+-----------------------------------------------------------------------------------│+----------+----------------------------------------------------------------------------------
-----------------------------------------------------------------------------------------------│----------------------------------------------------------------------------------------------
------------------------------------------------+                                              │--------------------------------------------------+
| Table    | Create Table                                                                      │| Table    | Create Table                                                                     
                                                                                               │                                                                                              
                                                |                                              │                                                  |
+----------+-----------------------------------------------------------------------------------│+----------+----------------------------------------------------------------------------------
-----------------------------------------------------------------------------------------------│----------------------------------------------------------------------------------------------
------------------------------------------------+                                              │--------------------------------------------------+
| accounts | CREATE TABLE `accounts` (                                                         │| accounts | CREATE TABLE `accounts` (
  `id` int(11) NOT NULL,                                                                       │  `id` int(11) NOT NULL,
  `balance` bigint(20) DEFAULT NULL,                                                           │  `balance` bigint(20) DEFAULT NULL,
  `aa` int(11) DEFAULT NULL,                                                                   │  `aa` int(11) DEFAULT NULL,
  `bb` int(11) DEFAULT 10,                                                                     │  `bb` int(11) DEFAULT 10,
  PRIMARY KEY (`id`)                                                                           │  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |                                  │) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |
+----------+-----------------------------------------------------------------------------------│+----------+----------------------------------------------------------------------------------
-----------------------------------------------------------------------------------------------│----------------------------------------------------------------------------------------------
------------------------------------------------+                                              │--------------------------------------------------+
1 row in set (0.051 sec)                                                                       │1 row in set (0.124 sec)
                                                                                               │

can you describe the detailed step?

you06 · 2020-11-03T07:41:08Z

@july2993 I ran the following SQLs from upstream

/* init */ drop table if exists accounts;
/* init */ create table accounts(id int primary key, balance bigint, aa int);
/* init */ insert into accounts values(1, 100, 1), (2, 0, 2), (3, 1, 3);

/* TXN1 */ set session tidb_enable_amend_pessimistic_txn = 1;
/* DDL */ set session tidb_enable_amend_pessimistic_txn = 1;
/* TXN1 */ begin;
/* DDL */ alter table accounts add column bb int default 10;
/* TXN1 */ update accounts set balance = id where 1;
/* TXN1 */ commit;
/* TXN1 */ select * from accounts;

drainer/translator/mysql.go

lichunzhu

Should we add an integration test for this case?

drainer/translator/translator.go

you06 · 2020-11-03T08:08:37Z

Tested with this PR got unexpected NULL value in down stream

MySQL [test]> select * from accounts;                                                          │MySQL [test]> select * from accounts;
+----+---------+------+------+                                                                 │+----+---------+------+------+
| id | balance | aa   | bb   |                                                                 │| id | balance | aa   | bb   |
+----+---------+------+------+                                                                 │+----+---------+------+------+
|  1 |       1 |    1 |   10 |                                                                 │|  1 |       1 |    1 | NULL |
|  2 |       2 |    2 |   10 |                                                                 │|  2 |       2 |    2 | NULL |
|  3 |       3 |    3 |   10 |                                                                 │|  3 |       3 |    3 | NULL |
+----+---------+------+------+                                                                 │+----+---------+------+------+
3 rows in set (0.063 sec)                                                                      │3 rows in set (0.098 sec)
                                                                                               │
MySQL [test]> show create table accounts;                                                      │MySQL [test]> show create table accounts;
+----------+-----------------------------------------------------------------------------------│+----------+----------------------------------------------------------------------------------
-----------------------------------------------------------------------------------------------│----------------------------------------------------------------------------------------------
------------------------------------------------+                                              │--------------------------------------------------+
| Table    | Create Table                                                                      │| Table    | Create Table                                                                     
                                                                                               │                                                                                              
                                                |                                              │                                                  |
+----------+-----------------------------------------------------------------------------------│+----------+----------------------------------------------------------------------------------
-----------------------------------------------------------------------------------------------│----------------------------------------------------------------------------------------------
------------------------------------------------+                                              │--------------------------------------------------+
| accounts | CREATE TABLE `accounts` (                                                         │| accounts | CREATE TABLE `accounts` (
  `id` int(11) NOT NULL,                                                                       │  `id` int(11) NOT NULL,
  `balance` bigint(20) DEFAULT NULL,                                                           │  `balance` bigint(20) DEFAULT NULL,
  `aa` int(11) DEFAULT NULL,                                                                   │  `aa` int(11) DEFAULT NULL,
  `bb` int(11) DEFAULT 10,                                                                     │  `bb` int(11) DEFAULT 10,
  PRIMARY KEY (`id`)                                                                           │  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |                                  │) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |
+----------+-----------------------------------------------------------------------------------│+----------+----------------------------------------------------------------------------------
-----------------------------------------------------------------------------------------------│----------------------------------------------------------------------------------------------
------------------------------------------------+                                              │--------------------------------------------------+
1 row in set (0.051 sec)                                                                       │1 row in set (0.124 sec)
                                                                                               │

It's caused by getDefaultOrZeroValue function will not populate default value column.

july2993 · 2020-11-04T07:09:21Z

/run-all-tests tidb=release-4.0 tikv=release-4.0 pd=release-4.0

july2993 · 2020-11-04T07:11:49Z

/run-all-tests tidb=release-4.0 tikv=release-4.0 pd=release-4.0

july2993 · 2020-11-04T07:15:25Z

/run-all-tests tidb=release-4.0 tikv=release-4.0 pd=release-4.0

july2993 · 2020-11-04T07:57:27Z

Should we add an integration test for this case?

yes, current test using a temporal tool https://github.com/july2993/tk/blob/4465f27496e7c6d8c509e8fde75bfce070c8c4b3/cmd/amend/amend.go#L42
we can port it if this solution if fine

you06 · 2020-11-04T08:09:25Z

I tried this case and got error

/* init */ drop table if exists accounts;
/* init */ create table accounts(id int primary key, balance bigint);
/* init */ insert into accounts values(1, 100), (2, 0), (3, 1);

/* TXN */ set session tidb_enable_amend_pessimistic_txn = 1;
/* DDL */ set session tidb_enable_amend_pessimistic_txn = 1;
/* TXN */ begin;
/* DDL */ alter table accounts add column bb int not null;
/* DDL */ alter table accounts add column cc int not null;
/* TXN */ update accounts set balance = id where 1;
/* TXN */ commit;
/* TXN */ select * from accounts;

[2020/11/04 08:08:34.838 +00:00] [ERROR] [executor.go:111] ["Exec fail, will rollback"] [query="REPLACE INTO `test`.`accounts`(`balance`,`bb`,`id`) VALUES(?,?,?)"] [args="[3,0,3]"] [error="Error 1364: Field 'cc' doesn't have a default value"]

july2993 · 2020-11-04T08:47:35Z

/run-all-tests tidb=release-4.0 tikv=release-4.0 pd=release-4.0

drainer/schema.go

july2993 · 2020-11-11T05:10:44Z

/run-all-tests tidb=release-4.0 tikv=release-4.0 pd=release-4.0

csuzhangxc

LGTM

csuzhangxc · 2020-11-11T06:23:45Z

@you06 PTAL again?

you06 · 2020-11-11T06:51:47Z

I'm running some tests on the latest commit.

zyguan · 2020-11-14T09:13:12Z

I did some tests today, there is a problem with enum type. The test case is

/* init */ drop table if exists t;
/* init */ create table t (id int primary key, c int);
/* init */ insert into t (id, c) values (1, 1);

/* txn */ begin;
/* txn */ update t set c = 2 where id = 1;
/* ddl */ alter table t add column cc enum('a','b','c','d','e') default 'b';
/* txn */ commit;

Drainer will execute REPLACE INTO test.t(c, cc, id) VALUES(2,0,1) on downstream, which will be rejected with Error 1265: Data truncated for column 'cc' . Drainer retried the txn for several times and finally exit with error. That is, we got a wrong default value for this enum column (Datum.b is "b" but Datum.i is 0).

july2993 · 2020-11-16T03:19:28Z

I did some tests today, there is a problem with enum type. The test case is
/* init */ drop table if exists t;
/* init */ create table t (id int primary key, c int);
/* init */ insert into t (id, c) values (1, 1);

/* txn */ begin;
/* txn */ update t set c = 2 where id = 1;
/* ddl */ alter table t add column cc enum('a','b','c','d','e') default 'b';
/* txn */ commit;
Drainer will execute REPLACE INTO test.t(c, cc, id) VALUES(2,0,1) on downstream, which will be rejected with Error 1265: Data truncated for column 'cc' . Drainer retried the txn for several times and finally exit with error. That is, we got a wrong default value for this enum column (Datum.b is "b" but Datum.i is 0).

fix in 18393fe

you06 · 2020-11-17T03:46:11Z

drainer/translator/table_info.go

-	IsDroppingColumn(id int64) bool
+	CanAppendDefaultValue(id int64, schemaVersion int64) bool
+	// IsDroppingColumn(id int64) bool
+	TableBySchemaVersion(version int64, tid int64) (info *model.TableInfo, ok bool)


The variable name here is a bit confusing.

id and tid are both table's id.

version and schemaVersion are both schema's version.

the order of id and version is not same.

you06 · 2020-11-18T13:31:35Z

LGTM

ti-srebot · 2020-11-18T13:31:37Z

@you06, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. See the corresponding SIG page for more information. Related SIG: migrate(slack).

july2993 · 2020-11-18T13:41:43Z

/run-all-tests tidb=release-4.0 tikv=release-4.0 pd=release-4.0

csuzhangxc

LGTM

Add default value when generate the update rows

eff40ea

only on the condition PreWrite.schemaVersion < schemaVersion of the table replicated to downstream.

july2993 added the status/PTAL label Nov 3, 2020

you06 reviewed Nov 3, 2020

View reviewed changes

drainer/translator/mysql.go Outdated Show resolved Hide resolved

rename sign

c1da936

lichunzhu reviewed Nov 3, 2020

View reviewed changes

you06 reviewed Nov 3, 2020

View reviewed changes

drainer/translator/translator.go Outdated Show resolved Hide resolved

cfzjywxk mentioned this pull request Nov 3, 2020

Transaction amender will cause failure of drainer pingcap/tidb#20773

Closed

Get the default value with the schemaVersion the txn use

8522482

go fmt simplify

35d0207

Fix check

37d2795

july2993 requested a review from lichunzhu November 4, 2020 07:57

Add multi cols missing

7b363ca

csuzhangxc reviewed Nov 5, 2020

View reviewed changes

drainer/schema.go Show resolved Hide resolved

july2993 added 3 commits November 10, 2020 22:56

add log

ce8443e

update dep

e9a49cd

change timestamp default value tz

91cc776

csuzhangxc reviewed Nov 11, 2020

View reviewed changes

csuzhangxc added the status/LGT1 label Nov 11, 2020

Fix enum&set default value

18393fe

you06 reviewed Nov 17, 2020

View reviewed changes

keep code style

b95e7eb

july2993 requested review from csuzhangxc and removed request for lichunzhu November 18, 2020 14:00

csuzhangxc approved these changes Nov 19, 2020

View reviewed changes

csuzhangxc added status/LGT2 and removed status/LGT1 status/PTAL labels Nov 19, 2020

july2993 merged commit 336b519 into pingcap:master Nov 19, 2020

july2993 deleted the amend branch November 19, 2020 02:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default value when generate the update rows #1006

Add default value when generate the update rows #1006

july2993 commented Nov 3, 2020 •

edited

Loading

july2993 commented Nov 3, 2020

you06 commented Nov 3, 2020

july2993 commented Nov 3, 2020

you06 commented Nov 3, 2020

lichunzhu left a comment

you06 commented Nov 3, 2020

july2993 commented Nov 4, 2020

july2993 commented Nov 4, 2020

july2993 commented Nov 4, 2020

july2993 commented Nov 4, 2020

you06 commented Nov 4, 2020

july2993 commented Nov 4, 2020

july2993 commented Nov 11, 2020

csuzhangxc left a comment

csuzhangxc commented Nov 11, 2020

you06 commented Nov 11, 2020

zyguan commented Nov 14, 2020 •

edited

Loading

july2993 commented Nov 16, 2020

you06 Nov 17, 2020

you06 commented Nov 18, 2020

ti-srebot commented Nov 18, 2020

july2993 commented Nov 18, 2020

csuzhangxc left a comment

Add default value when generate the update rows #1006

Add default value when generate the update rows #1006

Conversation

july2993 commented Nov 3, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

july2993 commented Nov 3, 2020

you06 commented Nov 3, 2020

july2993 commented Nov 3, 2020

you06 commented Nov 3, 2020

lichunzhu left a comment

Choose a reason for hiding this comment

you06 commented Nov 3, 2020

july2993 commented Nov 4, 2020

july2993 commented Nov 4, 2020

july2993 commented Nov 4, 2020

july2993 commented Nov 4, 2020

you06 commented Nov 4, 2020

july2993 commented Nov 4, 2020

july2993 commented Nov 11, 2020

csuzhangxc left a comment

Choose a reason for hiding this comment

csuzhangxc commented Nov 11, 2020

you06 commented Nov 11, 2020

zyguan commented Nov 14, 2020 • edited Loading

july2993 commented Nov 16, 2020

you06 Nov 17, 2020

Choose a reason for hiding this comment

you06 commented Nov 18, 2020

ti-srebot commented Nov 18, 2020

july2993 commented Nov 18, 2020

csuzhangxc left a comment

Choose a reason for hiding this comment

july2993 commented Nov 3, 2020 •

edited

Loading

zyguan commented Nov 14, 2020 •

edited

Loading