import: check table empty skips using index #39604

dsdashun · 2022-12-04T04:42:14Z

What problem does this PR solve?

Issue Number: close #39477

Problem Summary:

What is changed and how it works?

When pre-checking whether target table is empty, skip using the index and directly scan the record.
Added integration tests to verify that.

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 refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2022-12-04T04:42:16Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

D3Hunter
lance6716

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.

dsdashun · 2022-12-04T04:42:37Z

/run-integration-br-test

dsdashun · 2022-12-04T06:34:32Z

Integration BR test failed at br_foreign_key. All Lightning tests have passed: https://ci.pingcap.net/blue/organizations/jenkins/br_ghpr_unit_and_integration_test/detail/br_ghpr_unit_and_integration_test/14309/pipeline

dsdashun · 2022-12-05T01:39:12Z

/run-unit-test

lance6716

please fix unit tests, rest lgtm

lance6716 · 2022-12-05T06:47:44Z

br/pkg/lightning/restore/get_pre_info.go

+		// In Lightning, if previous importing is halted half-way, it is possible that
+		// the data is partially imported, but the index data has not been imported.
+		// In this situation, if no hint is added, the SQL executor might fetch the record from index,
+		// which is empty.  This will result in missing check.


it's still possible that index data in partially imported.

it's still possible that index data in partially imported.

If index data is partially imported, table-empty check will report errors on the second import even if USE INDEX() is not used. So it will be figured out by the current table-empty-check logic.

dsdashun · 2022-12-05T09:22:08Z

/merge

ti-chi-bot · 2022-12-05T09:22:12Z

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

Commit hash: 7ebf3c3

D3Hunter · 2022-12-05T09:23:40Z

tried on tiup playground nightly(v6.5.0-alpha-nightly-20221205), cannot reproduce it, steps:

run lightning, data imported, but failed before import index
rm checkpoint
import again:

➜  lightning bin/tidb-lightning --config test-bool-csv.toml
Verbose debug logs will be written to tidb-lightning.log

+----+------------------------------------------------------------------------------------------------------------------------------------+-------------+--------+
|  # | CHECK ITEM                                                                                                                         | TYPE        | PASSED |
+----+------------------------------------------------------------------------------------------------------------------------------------+-------------+--------+
|  1 | Source csv files size is proper                                                                                                    | performance | true   |
+----+------------------------------------------------------------------------------------------------------------------------------------+-------------+--------+
|  2 | the checkpoints are valid                                                                                                          | critical    | true   |
+----+------------------------------------------------------------------------------------------------------------------------------------+-------------+--------+
|  3 | table schemas are valid                                                                                                            | critical    | true   |
+----+------------------------------------------------------------------------------------------------------------------------------------+-------------+--------+
|  4 | table(s) [`test`.`t`] are not empty                                                                                                | critical    | false  |
+----+------------------------------------------------------------------------------------------------------------------------------------+-------------+--------+

mysql> show create table test.t;
+-------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                                                                                                   |
+-------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| t     | CREATE TABLE `t` (
  `id` int(11) NOT NULL,
  `v` tinyint(1) DEFAULT NULL,
  PRIMARY KEY (`id`) /*T![clustered_index] CLUSTERED */
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |
+-------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.01 sec)

mysql> select 1 from test.t  limit 1;
+---+
| 1 |
+---+
| 1 |
+---+
1 row in set (0.01 sec)

D3Hunter · 2022-12-05T09:24:52Z

/run-integration-br-test

dsdashun · 2022-12-05T09:26:06Z

/hold

D3Hunter · 2022-12-05T11:06:47Z

tried on tiup playground nightly(v6.5.0-alpha-nightly-20221205), cannot reproduce it, steps:

run lightning, data imported, but failed before import index

rm checkpoint

import again:
.....

seems when table rows is small, tidb will use full-table-scan（previous case only has 4 rows)

mysql> explain select 1 from t limit 1;
+-----------------------------+---------+-----------+---------------+--------------------------------+
| id                          | estRows | task      | access object | operator info                  |
+-----------------------------+---------+-----------+---------------+--------------------------------+
| Projection_6                | 1.00    | root      |               | 1->Column#3                    |
| └─Limit_7                   | 1.00    | root      |               | offset:0, count:1              |
|   └─TableReader_11          | 1.00    | root      |               | data:Limit_10                  |
|     └─Limit_10              | 1.00    | cop[tikv] |               | offset:0, count:1              |
|       └─TableFullScan_9     | 1.00    | cop[tikv] | table:t       | keep order:false, stats:pseudo |
+-----------------------------+---------+-----------+---------------+--------------------------------+

dsdashun · 2022-12-05T13:06:48Z

/unhold

dsdashun · 2022-12-05T13:08:41Z

tried on tiup playground nightly(v6.5.0-alpha-nightly-20221205), cannot reproduce it, steps:

run lightning, data imported, but failed before import index

rm checkpoint

import again:
.....

You can use the data introduced in c0e0c3c , which is reproducible.

dsdashun · 2022-12-05T13:09:40Z

/run-integration-br-test

dsdashun · 2022-12-05T13:39:22Z

/merge

ti-chi-bot · 2022-12-05T13:39:26Z

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

Commit hash: 85052b1

sre-bot · 2022-12-05T14:08:49Z

TiDB MergeCI notify

🔴 Bad News! [1] CI still failing after this pr merged.
These failed integration tests don't seem to be introduced by the current PR.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci/integration-cdc-test	🔴 failed 1, success 39, total 40	23 min	Existing failure
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	19 min	Existing passed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 17 tests passed	18 min	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	6 min 40 sec	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	4 min 38 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	4 min 20 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	3 min 49 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 41 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	3 min 10 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

dsdashun · 2023-01-17T10:43:53Z

/cherry-pick release-6.1

ti-chi-bot · 2023-01-17T10:44:35Z

@dsdashun: new pull request created to branch release-6.1: #40667.

In response to this:

/cherry-pick release-6.1

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 ti-community-infra/tichi repository.

Signed-off-by: ti-chi-bot <[email protected]>

dsdashun · 2023-01-17T10:44:45Z

/cherry-pick release-6.5

ti-chi-bot · 2023-01-17T10:45:23Z

@dsdashun: new pull request created to branch release-6.5: #40669.

In response to this:

/cherry-pick release-6.5

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 ti-community-infra/tichi repository.

close #39477

close pingcap#39477

close #39477

import: check table empty skips using index

bccc7ee

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 4, 2022

dsdashun requested review from lance6716, lichunzhu and D3Hunter December 5, 2022 02:19

lance6716 approved these changes Dec 5, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 5, 2022

Merge branch 'master' into fix-39477

59da153

D3Hunter approved these changes Dec 5, 2022

View reviewed changes

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 Dec 5, 2022

dsdashun added 2 commits December 5, 2022 17:21

fix unit-tests

3551571

Merge remote-tracking branch 'origin/master' into fix_empty_check

7ebf3c3

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 Dec 5, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 5, 2022

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 5, 2022

dsdashun added 2 commits December 5, 2022 21:04

update the test data to be reproducable

c0e0c3c

Merge remote-tracking branch 'origin/master' into fix_empty_check

85052b1

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Dec 5, 2022

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 5, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 5, 2022

ti-chi-bot merged commit 35819ee into pingcap:master Dec 5, 2022

dsdashun deleted the fix-39477 branch December 5, 2022 13:40

ti-chi-bot mentioned this pull request Jan 17, 2023

import: check table empty skips using index (#39604) #40667

Merged

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jan 17, 2023

This is an automated cherry-pick of pingcap#39604

156ef1a

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Jan 17, 2023

import: check table empty skips using index (#39604) #40669

Merged

12 tasks

ti-chi-bot added a commit that referenced this pull request Jan 18, 2023

import: check table empty skips using index (#39604) (#40667)

77cea7a

close #39477

dsdashun added affects-6.5 This bug affects the 6.5.x(LTS) versions. and removed affects-6.5 This bug affects the 6.5.x(LTS) versions. labels Jan 20, 2023

GMHDBJD pushed a commit to GMHDBJD/tidb that referenced this pull request Feb 1, 2023

import: check table empty skips using index (pingcap#39604)

b848cbc

close pingcap#39477

GMHDBJD mentioned this pull request Feb 1, 2023

import: check table empty skips using index #40936

Closed

12 tasks

ti-chi-bot added a commit that referenced this pull request Feb 4, 2023

import: check table empty skips using index (#39604) (#40669)

698f90e

close #39477

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

import: check table empty skips using index #39604

import: check table empty skips using index #39604

dsdashun commented Dec 4, 2022

ti-chi-bot commented Dec 4, 2022 •

edited

Loading

dsdashun commented Dec 4, 2022

dsdashun commented Dec 4, 2022

dsdashun commented Dec 5, 2022

lance6716 left a comment

lance6716 Dec 5, 2022

dsdashun Dec 5, 2022

dsdashun commented Dec 5, 2022

ti-chi-bot commented Dec 5, 2022

D3Hunter commented Dec 5, 2022

D3Hunter commented Dec 5, 2022

dsdashun commented Dec 5, 2022

D3Hunter commented Dec 5, 2022 •

edited

Loading

dsdashun commented Dec 5, 2022

dsdashun commented Dec 5, 2022

dsdashun commented Dec 5, 2022

dsdashun commented Dec 5, 2022

ti-chi-bot commented Dec 5, 2022

sre-bot commented Dec 5, 2022

dsdashun commented Jan 17, 2023

ti-chi-bot commented Jan 17, 2023

dsdashun commented Jan 17, 2023

ti-chi-bot commented Jan 17, 2023

import: check table empty skips using index #39604

import: check table empty skips using index #39604

Conversation

dsdashun commented Dec 4, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Dec 4, 2022 • edited Loading

dsdashun commented Dec 4, 2022

dsdashun commented Dec 4, 2022

dsdashun commented Dec 5, 2022

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 Dec 5, 2022

Choose a reason for hiding this comment

dsdashun Dec 5, 2022

Choose a reason for hiding this comment

dsdashun commented Dec 5, 2022

ti-chi-bot commented Dec 5, 2022

D3Hunter commented Dec 5, 2022

D3Hunter commented Dec 5, 2022

dsdashun commented Dec 5, 2022

D3Hunter commented Dec 5, 2022 • edited Loading

dsdashun commented Dec 5, 2022

dsdashun commented Dec 5, 2022

dsdashun commented Dec 5, 2022

dsdashun commented Dec 5, 2022

ti-chi-bot commented Dec 5, 2022

sre-bot commented Dec 5, 2022

TiDB MergeCI notify

dsdashun commented Jan 17, 2023

ti-chi-bot commented Jan 17, 2023

dsdashun commented Jan 17, 2023

ti-chi-bot commented Jan 17, 2023

ti-chi-bot commented Dec 4, 2022 •

edited

Loading

D3Hunter commented Dec 5, 2022 •

edited

Loading