Skip to content

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

Merged
ti-chi-bot merged 1 commit intopingcap:masterfrom
YangKeao:submit-ttl-scan-task-to-table
Jan 13, 2023
Merged

ttl: submit ttl scan task to the system table#40422
ti-chi-bot merged 1 commit intopingcap:masterfrom
YangKeao:submit-ttl-scan-task-to-table

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.

Details

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

YangKeao commented Jan 10, 2023

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 Outdated
Copy link
Member Author

@YangKeao YangKeao Jan 10, 2023

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 Outdated
Copy link
Collaborator

@lcwangchao lcwangchao Jan 11, 2023

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

@YangKeao YangKeao Jan 11, 2023

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

YangKeao commented Jan 11, 2023

/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

YangKeao commented Jan 11, 2023

/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

YangKeao commented Jan 11, 2023

/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

YangKeao commented Jan 11, 2023

/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

ti-chi-bot commented Jan 13, 2023

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

DetailsCommit 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