PageStorage: Fix some bugs by JaySon-Huang · Pull Request #4212 · pingcap/tiflash

JaySon-Huang · 2022-03-09T13:16:49Z

What problem does this PR solve?

Issue Number: ref #3594

Problem Summary:

What is changed and how it works?

a5991b1 fix that if we start an instance of PageStorage without any write operatorations, calling gc will make it crash
c9904eb fix that restoring will read the checkpoint file infinitely; and we should remove the checkpoint file from all_files or we will read the checkpoint file twice

aef7c37 fix that if we do sth like this:
- put 1 -> entry1@BlobFile{id:1, offset=0x00}
- put 1 -> entry2@BlobFile{id:1, offset=0xFF}
- in memory gc remove entry1
- put 3 -> entry3@BlobFile{id:1, offset=0x0} <-- reuse the space @ BlobFile 1

when we restore blobstat, we can not know that entry1 is removed in memory. So we need to restore blob stat this way:

Restore the whole MVCCMap
Use the latest entry of each page id to restore the blob stats

1479d4a fix the problem for external page

DeltaTree may generate a dmf_100 and add external page 100 to PageStorage
Add reference 101 -> 100
Delete external page 100
The external page 100 is logically deleted, but the dmf_100 remains on disk, and we directly add a reference to the file id, so PageStorage see "Add reference 102 -> 100" after 100 get logical deleted.

e19947b
Remove a check that is not necessary, it does not help checking anything. create_ver.sequence does not represent the sequence of being-ref-entry, but the ref-entry itself. So it is allow.

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

None

ti-chi-bot · 2022-03-09T13:16:50Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

jiaqizho
lidezhu

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.

JaySon-Huang · 2022-03-09T13:17:39Z

/run-all-tests

sre-bot · 2022-03-09T13:27:13Z

Coverage for changed files

Filename                           Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
PageDirectory.cpp                      437                66    84.90%          30                 3    90.00%         981               191    80.53%         362                83    77.07%
PageDirectory.h                         20                 4    80.00%          20                 4    80.00%          89                15    83.15%           0                 0         -
PageDirectoryFactory.cpp                36                 8    77.78%           4                 0   100.00%         104                20    80.77%          42                 9    78.57%
PageEntriesEdit.h                       44                 6    86.36%          29                 4    86.21%         147                38    74.15%          14                 6    57.14%
WAL/WALReader.cpp                       71                 9    87.32%           9                 0   100.00%         173                13    92.49%          54                12    77.78%
WAL/serialize.cpp                       47                11    76.60%          15                 1    93.33%         190                41    78.42%          52                16    69.23%
WALStore.cpp                            80                50    37.50%           8                 1    87.50%         126                49    61.11%          38                25    34.21%
tests/gtest_page_directory.cpp        8494              1499    82.35%          46                 0   100.00%        1549                 7    99.55%        2672              1352    49.40%
tests/gtest_page_storage.cpp          2599               952    63.37%          31                 7    77.42%         701               154    78.03%         832               498    40.14%
tests/gtest_wal_store.cpp             1941               260    86.60%          11                 0   100.00%         450                12    97.33%         642               314    51.09%
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                13769              2865    79.19%         203                20    90.15%        4510               540    88.03%        4708              2315    50.83%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
16784      9448             43.71%    189173  95719        49.40%

full coverage report (for internal network access only)

jiaqizho · 2022-03-10T02:35:54Z

@JaySon-Huang could you add some description(about the bug) in this PR? I got a little confused.

JaySon-Huang · 2022-03-10T04:35:19Z

@jiaqizho Updated. This PR combines several bug fixes from different aspects. You may want to check them by the commit instead of the whole changes of this PR.

JaySon-Huang · 2022-03-10T05:52:57Z

/run-integration-test

JaySon-Huang · 2022-03-11T07:34:27Z

/run-integration-test

JaySon-Huang · 2022-03-14T03:52:45Z

/run-all-tests

jiaqizho

LTGM

JaySon-Huang · 2022-03-14T07:02:42Z

/run-all-tests

sre-bot · 2022-03-14T07:16:54Z

Coverage for changed files

Filename                                            Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/Mpp/MPPTunnel.h                                     8                 8     0.00%           8                 8     0.00%          14                14     0.00%           0                 0         -
Storages/Page/V3/PageDirectory.cpp                      441                68    84.58%          30                 3    90.00%         986               195    80.22%         364                84    76.92%
Storages/Page/V3/PageDirectory.h                         24                 6    75.00%          24                 6    75.00%         101                21    79.21%           0                 0         -
Storages/Page/V3/PageDirectoryFactory.cpp                36                 8    77.78%           4                 0   100.00%         104                20    80.77%          42                 9    78.57%
Storages/Page/V3/PageEntriesEdit.h                       54                10    81.48%          39                 8    79.49%         157                42    73.25%          14                 6    57.14%
Storages/Page/V3/WAL/WALReader.cpp                       71                 9    87.32%           9                 0   100.00%         173                13    92.49%          54                12    77.78%
Storages/Page/V3/WAL/serialize.cpp                       47                11    76.60%          15                 1    93.33%         190                41    78.42%          52                16    69.23%
Storages/Page/V3/WALStore.cpp                            80                50    37.50%           8                 1    87.50%         126                49    61.11%          38                25    34.21%
Storages/Page/V3/tests/gtest_page_directory.cpp        8494              1499    82.35%          46                 0   100.00%        1549                 7    99.55%        2672              1352    49.40%
Storages/Page/V3/tests/gtest_page_storage.cpp          2599               952    63.37%          31                 7    77.42%         704               154    78.12%         832               498    40.14%
Storages/Page/V3/tests/gtest_wal_store.cpp             1941               260    86.60%          11                 0   100.00%         450                12    97.33%         642               314    51.09%
Storages/Transaction/SchemaBuilder.cpp                  835               794     4.91%          46                42     8.70%        1030               958     6.99%         488               468     4.10%
Storages/Transaction/SchemaGetter.h                       1                 1     0.00%           1                 1     0.00%           1                 1     0.00%           0                 0         -
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                 14631              3676    74.88%         272                77    71.69%        5585              1527    72.66%        5198              2784    46.44%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
16861      9457             43.91%    189873  95858        49.51%

full coverage report (for internal network access only)

jiaqizho · 2022-03-14T08:54:51Z

@JaySon-Huang

dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp:42:22: error: unused variable ‘reader’ [-Werror=unused-variable]
     auto [wal, reader] = WALStore::create(file_provider, delegator);
                      ^

please add

(void)reader;

JaySon-Huang · 2022-03-14T09:02:16Z

@JaySon-Huang

dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp:42:22: error: unused variable ‘reader’ [-Werror=unused-variable]
     auto [wal, reader] = WALStore::create(file_provider, delegator);
                      ^

please add

(void)reader;

Done

JaySon-Huang · 2022-03-14T09:02:36Z

/merge

ti-chi-bot · 2022-03-14T09:02:37Z

@JaySon-Huang: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

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.

ti-chi-bot · 2022-03-14T09:02:43Z

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

Details

Commit hash: e014bc2

sre-bot · 2022-03-14T09:21:53Z

Coverage for changed files

Filename                                            Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/Mpp/MPPTunnel.h                                     8                 8     0.00%           8                 8     0.00%          14                14     0.00%           0                 0         -
Interpreters/Settings.h                                   1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Storages/Page/V3/PageDirectory.cpp                      441                68    84.58%          30                 3    90.00%         986               195    80.22%         364                84    76.92%
Storages/Page/V3/PageDirectory.h                         24                 6    75.00%          24                 6    75.00%         101                21    79.21%           0                 0         -
Storages/Page/V3/PageDirectoryFactory.cpp                36                 8    77.78%           4                 0   100.00%         105                20    80.95%          42                 9    78.57%
Storages/Page/V3/PageEntriesEdit.h                       54                10    81.48%          39                 8    79.49%         157                42    73.25%          14                 6    57.14%
Storages/Page/V3/WAL/WALReader.cpp                       71                 9    87.32%           9                 0   100.00%         173                13    92.49%          54                12    77.78%
Storages/Page/V3/WAL/serialize.cpp                       47                11    76.60%          15                 1    93.33%         190                41    78.42%          52                16    69.23%
Storages/Page/V3/WALStore.cpp                            69                39    43.48%           8                 1    87.50%         119                42    64.71%          32                19    40.62%
Storages/Page/V3/tests/entries_helper.h                  65                36    44.62%          10                 3    70.00%         238               111    53.36%          32                21    34.38%
Storages/Page/V3/tests/gtest_page_directory.cpp        8494              1499    82.35%          46                 0   100.00%        1549                 7    99.55%        2672              1352    49.40%
Storages/Page/V3/tests/gtest_page_storage.cpp          2599               952    63.37%          31                 7    77.42%         704               154    78.12%         832               498    40.14%
Storages/Page/V3/tests/gtest_wal_store.cpp             1941               260    86.60%          11                 0   100.00%         451                13    97.12%         642               314    51.09%
Storages/Transaction/SchemaBuilder.cpp                  835               794     4.91%          46                42     8.70%        1030               958     6.99%         488               468     4.10%
Storages/Transaction/SchemaGetter.h                       1                 1     0.00%           1                 1     0.00%           1                 1     0.00%           0                 0         -
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                 14686              3701    74.80%         283                80    71.73%        5819              1632    71.95%        5224              2799    46.42%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
16861      9457             43.91%    189868  95840        49.52%

full coverage report (for internal network access only)

ref pingcap#3594 Signed-off-by: ywqzzy <592838129@qq.com>

Signed-off-by: ywqzzy <592838129@qq.com> update. Signed-off-by: ywqzzy <592838129@qq.com> update. Signed-off-by: ywqzzy <592838129@qq.com> Split write and ingest throughput (pingcap#4245) close pingcap#4243 Signed-off-by: ywqzzy <592838129@qq.com> fix the problem that expired data was not recycled timely due to slow gc speed (pingcap#4224) close pingcap#4146 Signed-off-by: ywqzzy <592838129@qq.com> PageStorage: Fix some bugs (pingcap#4212) ref pingcap#3594 Signed-off-by: ywqzzy <592838129@qq.com> Make test of CreateTables using affected opts (pingcap#4239) close pingcap#4235 Signed-off-by: ywqzzy <592838129@qq.com> BlobStore: remove the old_ids, not need recycle the blob id. (pingcap#4247) ref pingcap#3594 Signed-off-by: ywqzzy <592838129@qq.com> support `PartitionTableScan` in TiFlash (pingcap#3876) close pingcap#3873 Signed-off-by: ywqzzy <592838129@qq.com> update. Signed-off-by: ywqzzy <592838129@qq.com> update. Signed-off-by: ywqzzy <592838129@qq.com> support last_day and dayofmonth pushdown to tiflash (pingcap#4183) close pingcap#4149 Signed-off-by: ywqzzy <592838129@qq.com>

ti-chi-bot added do-not-merge/needs-linked-issue 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. and removed do-not-merge/needs-linked-issue labels Mar 9, 2022

JaySon-Huang requested review from jiaqizho and lidezhu March 9, 2022 13:31

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

JaySon-Huang added 7 commits March 14, 2022 11:24

Fix upsert seri as put

fcbf102

Fix nullptr in background gc without any write op

a5991b1

Fix for finding checkpoint file

c9904eb

Fix bug for restoring entries to BlobStats

aef7c37

Keep files to archive directory

d37fc1e

Add test and fix bug for PageDirectory

1479d4a

Fix some bugs on PageDirectory

e19947b

JaySon-Huang force-pushed the fix_ps_v3 branch from 3e1e301 to e19947b Compare March 14, 2022 03:24

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

Merge branch 'master' into fix_ps_v3

1cbed24

ti-chi-bot added the do-not-merge/needs-linked-issue label Mar 14, 2022

jiaqizho approved these changes Mar 14, 2022

View reviewed changes

ti-chi-bot added status/LGT1 Indicates that a PR has LGTM 1. and removed do-not-merge/needs-linked-issue labels Mar 14, 2022

JaySon-Huang added 2 commits March 14, 2022 15:47

Disable archive for V3 by default

f71ff7a

Format files

089e580

lidezhu approved these changes Mar 14, 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 Mar 14, 2022

Ingore unused var

801ea76

Merge branch 'master' into fix_ps_v3

e014bc2

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

ti-chi-bot merged commit 3d75154 into pingcap:master Mar 14, 2022

JaySon-Huang deleted the fix_ps_v3 branch March 14, 2022 09:43

ywqzzy pushed a commit to ywqzzy/tiflash_1 that referenced this pull request Mar 14, 2022

PageStorage: Fix some bugs (pingcap#4212)

a49aa40

ref pingcap#3594 Signed-off-by: ywqzzy <592838129@qq.com>

Conversation

JaySon-Huang commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaySon-Huang commented Mar 9, 2022

Uh oh!

sre-bot commented Mar 9, 2022

Coverage for changed files

Coverage summary

Uh oh!

jiaqizho commented Mar 10, 2022

Uh oh!

JaySon-Huang commented Mar 10, 2022

Uh oh!

JaySon-Huang commented Mar 10, 2022

Uh oh!

JaySon-Huang commented Mar 11, 2022

Uh oh!

JaySon-Huang commented Mar 14, 2022

Uh oh!

jiaqizho left a comment

Choose a reason for hiding this comment

Uh oh!

JaySon-Huang commented Mar 14, 2022

Uh oh!

sre-bot commented Mar 14, 2022

Coverage for changed files

Coverage summary

Uh oh!

jiaqizho commented Mar 14, 2022

Uh oh!

JaySon-Huang commented Mar 14, 2022

Uh oh!

JaySon-Huang commented Mar 14, 2022

Uh oh!

ti-chi-bot commented Mar 14, 2022

Uh oh!

ti-chi-bot commented Mar 14, 2022

Uh oh!

sre-bot commented Mar 14, 2022

Coverage for changed files

Coverage summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JaySon-Huang commented Mar 9, 2022 •

edited

Loading

ti-chi-bot commented Mar 9, 2022 •

edited

Loading