Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete full db directory with purge-chain subcommand #1786

Merged
merged 4 commits into from
Oct 5, 2023

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Oct 3, 2023

Closes #1767

Until now the purge-chain command would only remove the full subfolder of the db folder. However there is also the parachains db that currently remains and can cause problems on node restart.

Example wiht old code:

polkadot purge-chain --database paritydb --base-path /tmp/some-folder
Are you sure to remove "/tmp/some-folder/chains/polkadot/paritydb/full"? [y/N]: y
"/tmp/some-folder/chains/polkadot/paritydb/full" removed.

In this case /tmp/some-folder/chains/polkadot/paritydb/parachains would remain and might cause problem on node restart because of version conflicts as described in #1767. After this PR the whole /tmp/some-folder/chains/polkadot/paritydb folder will be deleted.

@skunert skunert added the T0-node This PR/Issue is related to the topic “node”. label Oct 3, 2023
@skunert skunert requested a review from a team October 3, 2023 22:41
@arkpar
Copy link
Member

arkpar commented Oct 4, 2023

It would be better to pass config.data_path here. It specifically points to a root where all databases are located.

@skunert
Copy link
Contributor Author

skunert commented Oct 4, 2023

It would be better to pass config.data_path here. It specifically points to a root where all databases are located.

If we nuke the entire data_path folder both rocksdb and paritydb would be removed in contrast to current behaviour of just removing the specified one. Is this what you are proposing?

@arkpar
Copy link
Member

arkpar commented Oct 4, 2023

That's a good point. Still, in addition to the main database and the parachain db, there is other stuff that needs to be deleted. The statement store database added in paritytech/substrate#13701 in particular.
I suggest we deprecate --database option for the purge command and just delete everything in data_path.
@bkchr what do you think?

@bkchr
Copy link
Member

bkchr commented Oct 4, 2023

and just delete everything in data_path.

Hmm, this would then also mean that we delete the keystore. I don't think we should do this. We could maybe introduce a new cli command purge-data-dir or whatever. That makes clear that it deletes everything, but I would not assume that delete database kills the keys.

@arkpar
Copy link
Member

arkpar commented Oct 4, 2023

Alright, then this is good for now.

@bkchr bkchr enabled auto-merge (squash) October 5, 2023 13:13
@skunert
Copy link
Contributor Author

skunert commented Oct 5, 2023

There was actually a test that was testing that parachains db survived. It mentioned one should use purge-chain --parachains which does not exist. I am not sure there is a use case to delete these separately, cc @cheme.

@cheme
Copy link
Contributor

cheme commented Oct 5, 2023

There was actually a test that was testing that parachains db survived. It mentioned one should use purge-chain --parachains which does not exist. I am not sure there is a use case to delete these separately, cc @cheme.

I remember deleting the parachain db being useful in some cases, at the time the point was that this could be safely deleted and would be restored from peers while keeping the substrate db.
I am not exactly sure if it should be a command on itself, or manual deletion is enough (I only expect this action to be done to fix some specific issues). I really don't know if it make sense to a have this command as a maintenance/regular action CC\ @ordian ?

@bkchr bkchr merged commit d21113c into paritytech:master Oct 5, 2023
109 checks passed
@ordian
Copy link
Member

ordian commented Oct 5, 2023

In general, it's not safe for more than 1/3 of validators to delete parachains db folder (or at least not used to be), but deleting the substrate db should be ok as long as there's one (full) node with the latest block. This is probably the reason why we had this --parachains flag previously for an expert mode or testnets.

bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
Closes paritytech#1767 

Until now the `purge-chain` command would only remove the `full`
subfolder of the db folder. However there is also the `parachains` db
that currently remains and can cause problems on node restart.

Example wiht old code:
```
polkadot purge-chain --database paritydb --base-path /tmp/some-folder
Are you sure to remove "/tmp/some-folder/chains/polkadot/paritydb/full"? [y/N]: y
"/tmp/some-folder/chains/polkadot/paritydb/full" removed.
```
In this case `/tmp/some-folder/chains/polkadot/paritydb/parachains`
would remain and might cause problem on node restart because of version
conflicts as described in paritytech#1767. After this PR the whole
`/tmp/some-folder/chains/polkadot/paritydb` folder will be deleted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

node reports unsupported database version even with
5 participants