Unify/sync dump with authority store #3494

oxade · 2022-07-26T04:23:28Z

This PR is a follow up of original tool which makes it such that the DB debug tool code stays in sync with the authority store codebase. See
Due to demand for DB debugging, I will add dump as a native feature of typed_store, but in the meantime, this PR keeps things in order
I also added a test which ensures that every table in the authority store can indeed be debugged. This way
Cli behavior of the tool is unchanged.

Next steps:

Checkpoint and lock service dump
Use procedural macro to autogenerate debug features for dumping. This will reduce code duplication, but requires a bit more work.
Implement robust debugging in typed_store to reduce code duplication/re-implementation in Sui

oxade · 2022-07-26T04:24:27Z

crates/sui-core/src/authority/authority_store_tables.rs

+    // TODO: condense with macros
+    pub fn dump(&self, table_name: &str) -> anyhow::Result<BTreeMap<String, String>> {
+        Ok(match table_name {
+            OBJECTS_TABLE_NAME => {


Next steps: These will be auto-generated by a procedural macro for any table which used DBMap.

oxade · 2022-07-26T04:24:58Z

crates/sui-tool/src/db_tool/db_dump.rs

+    use crate::db_tool::db_dump::{dump_table, list_tables};
+
+    #[tokio::test]
+    async fn db_dump_population() -> Result<(), anyhow::Error> {


Note: This test ensures that any table in the data store can be dumped, which prevents folks from adding tables without adding them to the dump list. Once we auto-gen, we will not need this test

oxade · 2022-07-26T04:40:36Z

crates/sui-core/src/authority/authority_store_tables.rs

+        let executed_sequence =
+            DBMap::reopen(&db, Some(EXEC_SEQ_TABLE_NAME)).expect("Cannot open CF.");
+
+        let (


This opening and naming pattern is used in many places around the code. Investigating macros we can provide from mysten-infra to reduce the noise

crates/sui-core/src/authority.rs

lxfind · 2022-07-26T18:32:41Z

Thanks. How does open_cf_opts_secondary work? Does it eventually make a clone of the entire db on the fly and then read from it?

oxade · 2022-07-26T19:35:08Z

Thanks. How does open_cf_opts_secondary work? Does it eventually make a clone of the entire db on the fly and then read from it?

No it does not clone the whole db. It reads the files in the primary db path in a lock-free mode, but it additionally needs to store some a few info logs of its own. These are stored in the secondary path. To keep in sync with primary data changes, we use a catchup logic to apply the primary WAL to the secondary's memtables upon every dump request. This is because the secondary cannot see the primary's memtables as it's in a different memory space.
So the only things copied are info logs to storage, and some memtables to RAM.

crates/sui-core/src/authority/authority_store_tables.rs

velvia

Ok, so I realize now that this change is basically

Porting the older db_dump code into a store_tables
and separating out store_tables from authority_store.

I have two thoughts:

Sooner or later, I'm going to refactor most of these tables into separate stores like an ObjectStore, CertificateStore etc. So, while this move of tables to a separate StoreTables is functionally fine, I wonder if it would be better to minimize the changes now and keep dump() where it is without moving all the tables, as stuff will get refactored later anyways
See my other comment but it wouldn't be much more work to create a trait like DumpableDBMap which would make the code much cleaner without having to match on every table. I think it could be doable without macros....

Anyways my request for change mainly has to do with my other comment about limits.

velvia · 2022-07-26T21:14:03Z

crates/sui-core/src/authority/authority_store_tables.rs

+            OBJECTS_TABLE_NAME => {
+                self.objects.try_catch_up_with_primary()?;
+                self.objects
+                    .iter()


We need to put a limit here. This will work today, but when we have millions of TXes, that BTreeMap you're building will quickly run out of memory. I'd rather we quickly change the limits than build something which does not have limits and may OOM.

Also, it wouldn't be much work to put in a trait like DumpableDBMap in typed_store crate which implements a Dump function for each key and value. Then, we would not need the match, and any table that implements DumpableDBMap could easily be dumped. If the API is good, we also would not need to dump everything out.

We need to put a limit here. This will work today, but when we have millions of TXes, that BTreeMap you're building will quickly run out of memory. I'd rather we quickly change the limits than build something which does not have limits and may OOM.

Also, it wouldn't be much work to put in a trait like DumpableDBMap in typed_store crate which implements a Dump function for each key and value. Then, we would not need the match, and any table that implements DumpableDBMap could easily be dumped. If the API is good, we also would not need to dump everything out.

For the trait, yes that's the plan. This will be introduced as we add support to more

Ok, so I realize now that this change is basically

Porting the older db_dump code into a store_tables

and separating out store_tables from authority_store.

I have two thoughts:

Sooner or later, I'm going to refactor most of these tables into separate stores like an ObjectStore, CertificateStore etc. So, while this move of tables to a separate StoreTables is functionally fine, I wonder if it would be better to minimize the changes now and keep dump() where it is without moving all the tables, as stuff will get refactored later anyways

See my other comment but it wouldn't be much more work to create a trait like DumpableDBMap which would make the code much cleaner without having to match on every table. I think it could be doable without macros....

Anyways my request for change mainly has to do with my other comment about limits.

The reason for this code change is to reduce code duplication. There are many improvements to be made, but this PR addresses just a slither.
I agree we should factor out the stores. But for now we can go incrementally rather than wait for everything.

Regarding using a trait, yes that was the initial plan and it's a good idea but it still puts dump logic in Sui. I decided this is a feature we will want in narhwal too so best if its part of typedstore natively

Keep in mind this PR is not the final form of the debug utility. It is merely a first evolution in our ability to support debugging features.

For limits, you want us to limit how much data we dump?
it makes sense but we're not at that point yet where its a concern I think
I can rather add pagination in the next PR, but this is not the core of this PR.

It's just we want to avoid having data structures that can grow unbounded as a principle.
Just .iter().take(LIMIT) should suffice for now.

We can discuss pagination later, agreed that should be pushed off.

This PR will be used to track the size limit improvement
#3532

Co-authored-by: Lu Zhang <8418040+longbowlu@users.noreply.github.com>

velvia

@oxade will create PR with follow on issues

oxade · 2022-07-26T22:29:16Z

@oxade will create PR with follow on issues

#3532

* Unify store and dump

oxade added 4 commits July 26, 2022 00:05

Unify store and dump

3202f3a

sync to main

a21b2aa

sync to main

00dbbdf

Comments

0078775

oxade commented Jul 26, 2022

View reviewed changes

oxade requested review from lxfind, mystenmark, longbowlu, patrickkuo and velvia July 26, 2022 04:25

oxade mentioned this pull request Jul 26, 2022

Ensure that db dump tool is always in-sync with the store structure #3359

Closed

oxade commented Jul 26, 2022

View reviewed changes

oxade enabled auto-merge (squash) July 26, 2022 06:32

oxade disabled auto-merge July 26, 2022 06:46

patrickkuo reviewed Jul 26, 2022

View reviewed changes

crates/sui-core/src/authority.rs Outdated Show resolved Hide resolved

typos in comments and logs

00d6aa0

oxade requested a review from patrickkuo July 26, 2022 16:05

longbowlu approved these changes Jul 26, 2022

View reviewed changes

crates/sui-core/src/authority/authority_store_tables.rs Outdated Show resolved Hide resolved

crates/sui-core/src/authority/authority_store_tables.rs Outdated Show resolved Hide resolved

velvia suggested changes Jul 26, 2022

View reviewed changes

oxade and others added 2 commits July 26, 2022 17:32

Update crates/sui-core/src/authority/authority_store_tables.rs

fe5d505

Co-authored-by: Lu Zhang <8418040+longbowlu@users.noreply.github.com>

typo

758e748

oxade requested a review from velvia July 26, 2022 21:45

typo

d3d0f3b

oxade enabled auto-merge (squash) July 26, 2022 21:50

velvia approved these changes Jul 26, 2022

View reviewed changes

oxade merged commit 855f882 into main Jul 26, 2022

oxade deleted the unify_dump_with_authority_store branch July 26, 2022 22:28

punwai pushed a commit that referenced this pull request Jul 27, 2022

Unify/sync dump with authority store (#3494)

89c0be3

* Unify store and dump

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify/sync dump with authority store #3494

Unify/sync dump with authority store #3494

oxade commented Jul 26, 2022 •

edited

Loading

oxade Jul 26, 2022 •

edited

Loading

oxade Jul 26, 2022 •

edited

Loading

oxade Jul 26, 2022

lxfind commented Jul 26, 2022

oxade commented Jul 26, 2022 •

edited

Loading

velvia left a comment

velvia Jul 26, 2022

oxade Jul 26, 2022 •

edited

Loading

oxade Jul 26, 2022 •

edited

Loading

velvia Jul 26, 2022

oxade Jul 26, 2022

velvia left a comment

oxade commented Jul 26, 2022

Unify/sync dump with authority store #3494

Unify/sync dump with authority store #3494

Conversation

oxade commented Jul 26, 2022 • edited Loading

oxade Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

oxade Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

oxade Jul 26, 2022

Choose a reason for hiding this comment

lxfind commented Jul 26, 2022

oxade commented Jul 26, 2022 • edited Loading

velvia left a comment

Choose a reason for hiding this comment

velvia Jul 26, 2022

Choose a reason for hiding this comment

oxade Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

oxade Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

velvia Jul 26, 2022

Choose a reason for hiding this comment

oxade Jul 26, 2022

Choose a reason for hiding this comment

velvia left a comment

Choose a reason for hiding this comment

oxade commented Jul 26, 2022

oxade commented Jul 26, 2022 •

edited

Loading

oxade Jul 26, 2022 •

edited

Loading

oxade Jul 26, 2022 •

edited

Loading

oxade commented Jul 26, 2022 •

edited

Loading

oxade Jul 26, 2022 •

edited

Loading

oxade Jul 26, 2022 •

edited

Loading