Enhance the check command to support diagnosing a subtree

[ahrtr]

The whole db is indexed using b+tree. The existing check functionality checks the whole db, naming the whole tree. It would be useful if it supports checking a sub-tree, naming support checking starting from a pageId or pageId + elementId.

Based on this new feature, we will be able to know whether any sub-tree is healthy or corrupted.

[ishan16696]

Hi @ahrtr,
I can work on this, would you like to assign this issue to me ?

[ahrtr]

Thanks @ishan16696

[ahrtr]

@ishan16696 any update on this?

[ishan16696]

Hi @ahrtr ,
I've started working on this but I require some time to deliver this feature.

[ishan16696]

naming support checking starting from a pageId or pageId + elementId.

@ahrtr do you want to take pageID as an argument ?

bbolt check [Path to db] [PageID]

[ahrtr]

For the CMD, let's add two optional flags --pageId and --elementIdx,

bbolt check path-2-db --pageId <pageId> --elementIdx <elementIdx>

If users do not specify the pageId and elementIdx, then the default behaviour is still checking the whole db file. Also note that it doesn't make sense only specifying --elementIdx if not specifying --pageId.

For the (*Tx) Check, please get the new options/parameters included in the CheckOption, so as to be backward compatible.

[ishan16696]

Also note that it doesn't make sense only specifying --elementIdx if not specifying --pageId.

make sense, at the same only specifying the --pageId can work ... right ?

[ahrtr]

at the same only specifying the --pageId can work ... right ?

YES. If only --pageId is specified, then we need to check all elements in the page.

[ahrtr]

any update? Please let me know if you need my assistance?

[ishan16696]

yes, I have made some progress. I have plan to work on this feature this weekend.

Please let me know if you need my assistance?

thanks, I will try to open a draft PR and you can have a look.

[ahrtr]

Discussed with @ishan16696 offline, I will take over the task and @ishan16696 will continue to work on the cmd change.

• The first step: Refactor the implementation of check #651
• Enhance the check method to support checking starting from a pageId and an elementIdx (@ahrtr will provide a draft PR)

  bbolt/tx_check.go

  Line 34 in 47cc272

  func (tx *Tx) check(kvStringer KVStringer, ch chan error) {

• Add test case to verify the change. @Elbehery can you take care of this ?
  • Create a db file, which contains at least one branch page, and multiple leaf pages. Intentionally corrupt one of the leaf page. Verify that checking on the corrupted page will fail, while other pages should succeed
• Update cmd side change. Also add sanity test case. @ishan16696 will take care of it.

[Elbehery]

Shall i cherry pick this PR and continue atop of it ?

[ahrtr]

• Refactor the implementation of check Refactor the implementation of check #651

We can merge #651 directly after it being reviewed. I will raise another draft PR, then you can cherry-pick it.

[ahrtr]

• Update cmd side change. Also add sanity test case. @ishan16696 will take care of it.

@ishan16696 Please feel free to deliver a PR to update the cmd. thx

[ahrtr]

@ishan16696 are you still interested working on this? I will take over if there is no response in this week. thx

[Elbehery]

@ahrtr i can continue working on it

[ahrtr]

@ahrtr i can continue working on it

thx. Let's wait for @ishan16696 's response firstly, if there is no response this week, then please feel free to raise a PR to update the CLI.

[ishan16696]

thx. Let's wait for @ishan16696 's response firstly, if there is no response this week, then please feel free to raise a PR to update the CLI.

I have open a PR. Please have a look.

[ahrtr]

Does anyone have bandwidth and be interested in resolve comment below?

#698 (comment)

cc @Elbehery @ivanvc

[Elbehery]

@ahrtr on me 👍🏽

[ivanvc]

@ahrtr, are we fine re-implementing as a cobra-style command? We know that pflags are not compatible with Go-native flags (as they're not POSIX compliant).

[ahrtr]

@ahrtr, are we fine re-implementing as a cobra-style command? We know that pflags are not compatible with Go-native flags (as they're not POSIX compliant).

It should be ok as long as there is no any behaviour change from users' perspective.

[Elbehery]

@ahrtr hello ✋🏽

Shall i work on this or @ivanvc will do ?

[ahrtr]

@ahrtr hello ✋🏽

Shall i work on this or @ivanvc will do ?

Please feel free to work on it, since @ivanvc already thumbed up your comment above #580 (comment)

[ivanvc]

Sorry for the confusion @Elbehery. Yes, my thumbs up meant that I ACK'd that you would work on it. I just wanted to confirm the decision to migrate the command to cobra-style ✌️

[Elbehery]

i rasied #723 to fix #580 (comment)

[ahrtr]

We still need to enhance the bbolt check command to support the flag --pageId. Please refer to #698 (comment)

cc @Elbehery @ivanvc

[Elbehery]

@ahrtr raised #737

[ahrtr]

All done. Thanks all.