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

ttl: submit ttl scan task to the system table #40422

Merged
merged 1 commit into from
Jan 13, 2023
Merged

ttl: submit ttl scan task to the system table #40422

merged 1 commit into from
Jan 13, 2023

Conversation

YangKeao
Copy link
Member

@YangKeao YangKeao commented Jan 9, 2023

Signed-off-by: YangKeao yangkeao@chunibyo.icu

What problem does this PR solve?

Issue Number: close #40362, #40363

Problem Summary:

This PR creates a system table to track the TTL scan tasks, and add some utilities to insert / delete the scan tasks.

This PR also submits and removes the related ttl scan tasks when the job is finished.

Check List

Tests

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

@YangKeao YangKeao requested a review from lcwangchao January 9, 2023 08:53
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jan 9, 2023 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • bb7133
  • lcwangchao

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.

@YangKeao YangKeao requested a review from a team as a code owner January 9, 2023 08:53
@ti-chi-bot ti-chi-bot added needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 9, 2023
@YangKeao YangKeao force-pushed the submit-ttl-scan-task-to-table branch from 0f1b28b to 519a625 Compare January 9, 2023 08:53
@YangKeao YangKeao added the release-note-none Denotes a PR that doesn't merit a release note. label Jan 9, 2023
@ti-chi-bot ti-chi-bot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jan 9, 2023
@YangKeao YangKeao force-pushed the submit-ttl-scan-task-to-table branch 3 times, most recently from 1352e51 to 525d0b8 Compare January 9, 2023 10:03
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 9, 2023
@YangKeao YangKeao removed the needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. label Jan 9, 2023
@YangKeao
Copy link
Member Author

YangKeao commented Jan 9, 2023

The failed test is really wired. The SELECT xxx FROM mysql.tidb_ttl_table_status panic. But if I remove the lines inserting data into the ttl_task, then it'll not occur. I cannot image what's the connection between these two queries...

This problem occurs in column "last_job_start_time", which should be NULL and default NULL. However, when selecting it, the decoder doesn't find out that it's nil, and cause the panic.

@YangKeao
Copy link
Member Author

As the debugger told me, the numNotNullCols + numNullCols doesn't equal with the total columns count 🤔 Really wired.

@YangKeao YangKeao force-pushed the submit-ttl-scan-task-to-table branch from 525d0b8 to 71635ea Compare January 10, 2023 13:12
YangKeao
YangKeao commented Jan 10, 2023
View reviewed changes
store/mockstore/unistore/tikv/mvcc.go
reqCtx.buf, err = encodeFromOldRow(m.Value, reqCtx.buf)
if err != nil {
log.Error("encode data failed", zap.Binary("value", m.Value), zap.Binary("key", m.Key), zap.Stringer("op", m.Op), zap.Error(err))
return nil, err
}

lock.Value = make([]byte, len(reqCtx.buf))
copy(lock.Value, reqCtx.buf)
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 finally find the bug! Reuse the reqCtx.buf could make the lock.Value be overwritten by another row.

It seems that this bug requires 1PC, and set multiple rows in one transaction (if these rows are in different tables, it will panic), so this problem only occurs in the TTL test, in which we set the session variables to enable 1PC explicitly.

It takes me a lot of time to reach here 😢 .

@YangKeao YangKeao force-pushed the submit-ttl-scan-task-to-table branch 2 times, most recently from a8770e5 to f2fe4bd Compare January 10, 2023 14:37
lcwangchao
lcwangchao reviewed Jan 11, 2023
View reviewed changes
store/mockstore/unistore/tikv/mvcc.go
@@ -935,16 +935,16 @@ func (store *MVCCStore) buildPrewriteLock(reqCtx *requestCtx, m *kvrpcpb.Mutatio
lock.Op = uint8(kvrpcpb.Op_Put)
}
if rowcodec.IsRowKey(m.Key) && lock.Op == uint8(kvrpcpb.Op_Put) {
if rowcodec.IsNewFormat(m.Value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When rowcodec.IsNewFormat(m.Value) , the previous code are deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here it sets reqCtx.buf = m.Value and later lock.Value = reqCtx.buf. The main purpose is to set lock.Value = m.Value. In https://github.com/pingcap/tidb/pull/40422/files/f2fe4bde5508cad89d582145709fa286a24bec18#diff-c8058e505bb9be555aca4f1a51fbf20e712ea4d6d89812a09f3ebf02ff99e2f2R896, the lock.Value has been initialized with m.Value, so there is no need to assign again.

@YangKeao
Copy link
Member Author

/retest

@YangKeao YangKeao force-pushed the submit-ttl-scan-task-to-table branch from f2fe4bd to 6bd6ef6 Compare January 11, 2023 05:48
@YangKeao
Copy link
Member Author

/hold I forgot to add "expireTime" column for ttl task!

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2023
@YangKeao YangKeao force-pushed the submit-ttl-scan-task-to-table branch from 6bd6ef6 to da85ab8 Compare January 11, 2023 08:31
@YangKeao
Copy link
Member Author

/unhold

@ti-chi-bot ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2023
@YangKeao
Copy link
Member Author

/retest

@YangKeao YangKeao force-pushed the submit-ttl-scan-task-to-table branch 2 times, most recently from d5ff369 to 93d9c89 Compare January 12, 2023 09:23
@YangKeao YangKeao mentioned this pull request Jan 13, 2023
Merged
12 tasks
@YangKeao
submit ttl scan task to the table
b1b5e2c 
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
@YangKeao YangKeao force-pushed the submit-ttl-scan-task-to-table branch from 93d9c89 to b1b5e2c Compare January 13, 2023 07:25
bb7133
bb7133 approved these changes Jan 13, 2023
View reviewed changes
Copy link
Member

@bb7133 bb7133 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-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 13, 2023
@bb7133
Copy link
Member

bb7133 commented Jan 13, 2023

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: b1b5e2c

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 13, 2023
@ti-chi-bot ti-chi-bot merged commit b619324 into pingcap:master Jan 13, 2023
@YangKeao YangKeao mentioned this pull request Jan 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcwangchao lcwangchao lcwangchao approved these changes

@bb7133 bb7133 bb7133 approved these changes

Assignees
No one assigned
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create ttl_tasks system table to record task
4 participants
@YangKeao @ti-chi-bot @bb7133 @lcwangchao