executor: optimize delete your write #28806

cfzjywxk · 2021-10-14T03:09:32Z

What problem does this PR solve?

Issue Number:
Related to #27564

Problem Summary:
The transaction mutation generation is incompatible with each other for different situations like optimistic/pessimistic transactions with constraint check in place or not.

What is changed and how it works?

What's Changed:
Try to use key node marks to differentiate the keys newly inserted and then deleted, see the comments in the issue above for details.

How it Works:

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please add a release note, or a 'None' if it is not needed.

ti-chi-bot · 2021-10-14T03:09:33Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

cfzjywxk · 2021-10-14T10:49:08Z

table/tables/tables.go

@@ -802,6 +802,7 @@ func (t *TableCommon) AddRecord(sctx sessionctx.Context, r []types.Datum, opts .
 			}
 		} else {
 			_, err = txn.Get(ctx, key)
+			setPresume = true


This change is used to make the behaviour same for optimistic transactions regardless of the config tidb_constraint_check_in_place. All the keys need existency check will go the Op_CheckNotExist path in commit.

you06 · 2021-10-15T07:46:56Z

I tried this case and still can get a del record in write CF.

/* s1 */ drop table if exists t;
/* s1 */ set session tidb_enable_clustered_index = 'off';
/* s1 */ create table t(id int primary key, v int);

/* s1 */ begin pessimistic;
/* s1 */ select * from t where id = 1 for update;
/* s1 */ insert into t values(1, 1);
/* s1 */ delete from t where id = 1;
/* s1 */ commit;

/* s1 */ select * from t; -- empty

» curl http://127.0.0.1:10080/mvcc/index/test/t/primary/1\?id\=1
 "key": "7480000000000000405F698000000000000001038000000000000001",
 "region_id": 2,
 "value": {
  "info": {
   "writes": [
    {
     "type": 1,
     "start_ts": 428417554420072454,
     "commit_ts": 428417554433179651
    }
   ]
  }
 }
}

With clustered-index on, the result is the same.

/* s1 */ drop table if exists t;
/* s1 */ set session tidb_enable_clustered_index = 'on';
/* s1 */ create table t(id int primary key, v int);

/* s1 */ begin pessimistic;
/* s1 */ select * from t where id = 1 for update;
/* s1 */ insert into t values(1, 1);
/* s1 */ delete from t where id = 1;
/* s1 */ commit;

/* s1 */ select * from t;

» curl http://127.0.0.1:10080/mvcc/key/test/t/1
{
 "key": "74800000000000003D5F728000000000000001",
 "region_id": 2,
 "value": {
  "info": {
   "writes": [
    {
     "type": 1,
     "start_ts": 428417709446791174,
     "commit_ts": 428417709446791177
    }
   ]
  }
 }
}

Because when adding a pessimistic lock, the node for the index key will is allocated.
https://github.com/tikv/client-go/blob/a7d8ea1587e085fa951389656998493e841bccf7/txnkv/transaction/txn.go#L670

cfzjywxk · 2021-10-15T08:26:51Z

I tried this case and still can get a del record in write CF.

/* s1 */ drop table if exists t;
/* s1 */ set session tidb_enable_clustered_index = 'off';
/* s1 */ create table t(id int primary key, v int);

/* s1 */ begin pessimistic;
/* s1 */ select * from t where id = 1 for update;
/* s1 */ insert into t values(1, 1);
/* s1 */ delete from t where id = 1;
/* s1 */ commit;

/* s1 */ select * from t; -- empty

» curl http://127.0.0.1:10080/mvcc/index/test/t/primary/1\?id\=1
 "key": "7480000000000000405F698000000000000001038000000000000001",
 "region_id": 2,
 "value": {
  "info": {
   "writes": [
    {
     "type": 1,
     "start_ts": 428417554420072454,
     "commit_ts": 428417554433179651
    }
   ]
  }
 }
}

With clustered-index on, the result is the same.

/* s1 */ drop table if exists t;
/* s1 */ set session tidb_enable_clustered_index = 'on';
/* s1 */ create table t(id int primary key, v int);

/* s1 */ begin pessimistic;
/* s1 */ select * from t where id = 1 for update;
/* s1 */ insert into t values(1, 1);
/* s1 */ delete from t where id = 1;
/* s1 */ commit;

/* s1 */ select * from t;

» curl http://127.0.0.1:10080/mvcc/key/test/t/1
{
 "key": "74800000000000003D5F728000000000000001",
 "region_id": 2,
 "value": {
  "info": {
   "writes": [
    {
     "type": 1,
     "start_ts": 428417709446791174,
     "commit_ts": 428417709446791177
    }
   ]
  }
 }
}

Because when adding a pessimistic lock, the node for the index key will is allocated. https://github.com/tikv/client-go/blob/a7d8ea1587e085fa951389656998493e841bccf7/txnkv/transaction/txn.go#L670

Good catch, I'm thinking about the solution, how to accurately identify the newly inserted value which must not exist in storage.

cfzjywxk · 2021-11-16T08:33:39Z

/run-all-tests

you06 · 2021-11-17T03:03:24Z

session/session_test.go

@@ -5813,3 +5813,51 @@ func (s *testSessionSuite) TestSameNameObjectWithLocalTemporaryTable(c *C) {
 			"  `cs1` int(11) DEFAULT NULL\n" +
 			") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin"))
 }
+
+func (s *testSessionSuite) TestOptimizedDeleteYourWrites(c *C) {


Seems this test also passes without optimization of delete your writes.

This is used to verify the optimization would not break consistency. To verify if the dangling delete is optimized, we may need to check the mvcc version and such test cases are easier to implement in utf.

you06 · 2021-11-17T04:00:00Z

sessionctx/stmtctx/stmtctx.go

@@ -197,6 +197,9 @@ type StatementContext struct {
 	EnableOptimizeTrace bool
 	// LogicalOptimizeTrace indicates the trace for optimize
 	LogicalOptimizeTrace *tracing.LogicalOptimizeTracer
+	// IsNewlyInserted indicates the current statement is inserting new rows. This flag will be
+	// used to optimize the `delete-your-write` situation.
+	IsNewlyInserted bool


Does this flag need to be reset when a new statement comes?

It's needed, a new statement may create a new stmtCtx object, but for StatementContext.ResetForRetry, seems we still need to reset it there?

Is the flag only for passing value from AddRecord to index.Create? I didn't understand 😕 What if insert on duplicate update two records, one of which already exists?

Yes, as the index.Create could be used by different upper functions such as ReBuildIndex we need to find out a way to classify if we are creating newly inserted index keys.

Is the flag only for passing value from AddRecord to index.Create? I didn't under stand 😕 What if insert on duplicate update two records, one of which already exists?

@MyonKeminta

Seems there's still some problems and it's not statement level flag and should be row level, thanks for reminding.

@you06 @MyonKeminta
PTAL again, the stmtCtx is removed and create options is used so that the affect is not statement level.

MyonKeminta · 2021-11-17T10:03:33Z

table/tables/index.go

+				err = txn.GetMemBuffer().SetWithFlags(key, idxVal, kv.SetPresumeKeyNotExists, kv.SetNewlyInserted)
+			} else {
+				err = txn.GetMemBuffer().SetWithFlags(key, idxVal, kv.SetPresumeKeyNotExists)
+			}


If in such a table:

create table t (id int primary key, v int, unique index (v)); insert into t values (1, 10);

If we execute:

begin; delete from t where id = 1; insert into t values (2, 10); delete from t where id = 2; commit;

Is it possible that the unique key is regarded as a delete-your-write and does noop on this transaction, results in the index key being left not deleted?

The newlyInserted flag is used with presumeNotExist flag which ensures the targe key does not exist both in memdb and the store. If the unique key is deleted first, the get operation in memdb will return zero length value and a nil error. So the flag will not use for this case.

ti-chi-bot · 2021-12-18T08:29:38Z

@cfzjywxk: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ti-chi-bot · 2023-12-19T16:36:24Z

@cfzjywxk: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-br-integration-test	`3f1bce8`	link	true	`/test pull-br-integration-test`
pull-integration-ddl-test	`3f1bce8`	link	true	`/test pull-integration-ddl-test`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

cfzjywxk added type/bugfix This PR fixes a bug. require-LGT3 Indicates that the PR requires three LGTM. sig/transaction SIG:Transaction labels Oct 14, 2021

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 14, 2021

cfzjywxk commented Oct 14, 2021

View reviewed changes

cfzjywxk requested review from sticnarf, longfangsong, you06, MyonKeminta and ekexium October 14, 2021 10:49

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2021

cfzjywxk force-pushed the optimize_delete_your_write branch from fc2a818 to 315aa95 Compare November 8, 2021 08:51

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2021

cfzjywxk force-pushed the optimize_delete_your_write branch from 315aa95 to eed0dff Compare November 16, 2021 03:27

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 16, 2021

cfzjywxk mentioned this pull request Nov 16, 2021

add newly inserted flag to optimize delete your write tikv/client-go#378

Merged

cfzjywxk force-pushed the optimize_delete_your_write branch from eed0dff to e46ec19 Compare November 16, 2021 08:28

you06 reviewed Nov 17, 2021

View reviewed changes

MyonKeminta reviewed Nov 17, 2021

View reviewed changes

you06 mentioned this pull request Nov 24, 2021

Query in transaction may return rows with same unique index column value #24195

Open

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2021

cfzjywxk requested review from MyonKeminta and you06 December 6, 2021 13:25

optimize delete your write and remove dangling commit record

3f1bce8

cfzjywxk force-pushed the optimize_delete_your_write branch from ce45f7c to 3f1bce8 Compare December 6, 2021 13:32

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2021

cfzjywxk requested a review from coocood December 6, 2021 13:34

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2021

ti-chi-bot bot deleted a comment from ti-chi-bot Dec 19, 2023

cfzjywxk closed this Dec 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: optimize delete your write #28806

executor: optimize delete your write #28806

cfzjywxk commented Oct 14, 2021 •

edited

Loading

ti-chi-bot commented Oct 14, 2021

cfzjywxk Oct 14, 2021

you06 commented Oct 15, 2021

cfzjywxk commented Oct 15, 2021 •

edited

Loading

cfzjywxk commented Nov 16, 2021

you06 Nov 17, 2021

cfzjywxk Nov 17, 2021

you06 Nov 17, 2021

cfzjywxk Nov 17, 2021

MyonKeminta Nov 17, 2021 •

edited

Loading

cfzjywxk Nov 17, 2021

cfzjywxk Nov 17, 2021

cfzjywxk Nov 17, 2021

MyonKeminta Nov 17, 2021

cfzjywxk Nov 17, 2021

ti-chi-bot commented Dec 18, 2021

ti-chi-bot bot commented Dec 19, 2023

executor: optimize delete your write #28806

executor: optimize delete your write #28806

Conversation

cfzjywxk commented Oct 14, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Oct 14, 2021

Choose a reason for hiding this comment

you06 commented Oct 15, 2021

cfzjywxk commented Oct 15, 2021 • edited Loading

cfzjywxk commented Nov 16, 2021

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

MyonKeminta Nov 17, 2021 • edited Loading

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

ti-chi-bot commented Dec 18, 2021

ti-chi-bot bot commented Dec 19, 2023

cfzjywxk commented Oct 14, 2021 •

edited

Loading

cfzjywxk commented Oct 15, 2021 •

edited

Loading

MyonKeminta Nov 17, 2021 •

edited

Loading