solana icon indicating copy to clipboard operation
solana copied to clipboard

Add fallback for ledger-tool commands to create new column families

Open steviez opened this issue 2 years ago • 5 comments

Problem

RocksDB settings include an option to create_if_missing, which will create missing columns or the entire rocksdb directory if starting from scratch. However, create_if_missing functionality only works if the session has Primary (read+write) access. Many ledger-tool commands only need Secondary (read-only) access to the database, so these commands are unable to open the Blockstore when a column must be added.

Summary of Changes

This change detects when Secondary access open fails due to missing column(s) or files, opens the database temporarily with Primary access, and then reattempts to open the database Secondary access.

steviez avatar Jul 11 '22 20:07 steviez

One thing to discuss is whether we want any background compaction happens naturally or open with background compaction disabled.

We had previously decided on no compactions with Secondary when we made these command read-only in the first place, and I don't think anything has changed that would alter that sentiment. Accordingly, I used PrimaryForMaintenance when I temporarily open the DB over Primary; this ensures no compactions.

steviez avatar Jul 12 '22 16:07 steviez

We had previously decided on no compactions with Secondary when we made these command read-only in the first place, and I don't think anything has changed that would alter that sentiment. Accordingly, I used PrimaryForMaintenance when I temporarily open the DB over Primary; this ensures no compactions.

Oh, oh, I meant when the new flag is on. Not sure if we have any occasion where we want the background compaction running. --force-open-blockstore sounds a little bit unclear to me as the user might not know how we will force open the blockstore (and the force open might still fail in some cases I guess).

For this, how about adding a flag to allow users to open the db with the specified mode: Secondary, PrimaryForMaintenance, or Primary? This gives users flexibility and clear ideas on what to expect.

yhchiang-sol avatar Jul 12 '22 16:07 yhchiang-sol

Oh, oh, I meant when the new flag is on. Not sure if we have any occasion where we want the background compaction running

With the flag on, we will only have the session open with Primary access very briefly; we close the Primary session immediately and then continue the command with Secondary. I don't think any useful amount of compactions could occur in this window, so I think we should keep it simple by choosing no-compactions by default

--force-open-blockstore sounds a little bit unclear to me as the user might not know how we will force open the blockstore (and the force open might still fail in some cases I guess).

I ended up going with another name, let's discuss that in the other comment.

For this, how about adding a flag to allow users to open the db with the specified mode: Secondary, PrimaryForMaintenance, or Primary? This gives users flexibility and clear ideas on what to expect.

Are you talking about allowing user to specify access for the temporary open, or for the entire subcommand? Either way, I think we know what the access type should be, and can pick accordingly. This would be an extra knob that I currently fail to see the value add of

steviez avatar Jul 15 '22 12:07 steviez

With the flag on, we will only have the session open with Primary access very briefly; we close the Primary session immediately and then continue the command with Secondary. I don't think any useful amount of compactions could occur in this window, so I think we should keep it simple by choosing no-compactions by default

I think I got the idea. I think it's a good idea to distinguish between temporary write access and primary write access. The former is to have minimum write access to open the DB and the type of write access (such as the one used in the copy command) where the ledger tool needs a longer period of write access.

Are you talking about allowing user to specify access for the temporary open, or for the entire subcommand? Either way, I think we know what the access type should be, and can pick accordingly. This would be an extra knob that I currently fail to see the value add of

Let's keep it simple and have the ledger tool decide the best access mode for the users.

yhchiang-sol avatar Jul 15 '22 14:07 yhchiang-sol

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] avatar Jul 31 '22 06:07 stale[bot]

Been a while since CI ran on this; just rebased for safety. Given the previous approval, will push on greens

steviez avatar Aug 12 '22 23:08 steviez