feat(index): add lotus shed commands to prune all indexes #12393

akaladarshi · 2024-08-15T13:47:15Z

Related Issues

Fixes: #12377

Proposed Changes

Add a lotus-shed command to prune the indexes, other
- Add a prune-txhash command to prune tx hash index
- Add a prune-events command
- refactor prune-msgindex command to prune the msg index
- Add prune-all-indexes command to prune all the indexes (msg, events, tx hash)at once
Use older-than flag to get the height to start pruning indexes older than the provided height

akaladarshi · 2024-08-15T13:49:41Z

@aarshkshah1992 We can also create separate commands for pruning the events, and txhash db separately.

Need suggestions on a couple of things:

Instead of going to each event one by one, this PR directly deletes based on the height and epochs
Use of go routines

rvagg · 2024-08-16T03:27:58Z

cmd/lotus-shed/indexes.go

+		&cli.IntFlag{
+			Name:  "from",
+			Value: 0,
+			Usage: "the tipset height to start backfilling from (0 is head of chain)",


"pruning", not "backfilling"?

rvagg · 2024-08-16T03:29:27Z

cmd/lotus-shed/indexes.go

+		if err := rows.Scan(&id, &height); err != nil {
+			return "", "", "", xerrors.Errorf("error scanning event id: %w", err)
+		}
+		eventIds = append(eventIds, id)


oh, this could get big, probably not ideal for an sql query

rvagg · 2024-08-16T03:36:06Z

I think this is going to be pretty inefficient doing the select and building a really long targeted delete, can we not just use a range delete? I'm also not sure it's worth the bother doing a full range with these (i.e. the "epochs" arg probably doesn't get used by anyone). I think you'd just want to have an "older than" (in fact, you should rename "from" to be something like that, --older-than?).

So, can we just bundle the heavy lifting directly into the queries?

DELETE FROM event_entry WHERE event_id IN (SELECT id FROM event WHERE height < ?);
DELETE FROM event  WHERE height < ?;
DELETE FROM events_seen WHERE height < ?;

akaladarshi · 2024-08-16T05:28:24Z

I think this is going to be pretty inefficient doing the select and building a really long targeted delete, can we not just use a range delete? I'm also not sure it's worth the bother doing a full range with these (i.e. the "epochs" arg probably doesn't get used by anyone). I think you'd just want to have an "older than" (in fact, you should rename "from" to be something like that, --older-than?).

So, can we just bundle the heavy lifting directly into the queries?
DELETE FROM event_entry WHERE event_id IN (SELECT id FROM event WHERE height < ?);
DELETE FROM event  WHERE height < ?;
DELETE FROM events_seen WHERE height < ?;

Yeah, you're correct range queries will be more efficient, will update it.
- I was thinking in the direction of how it was entered (event-> events
  _entries -> event_seen), but we can delete them directly (much more efficient)
As per the discussion we (me and @aarshkshah1992 ) had, we decided to include the epoch part, but as you're saying it won't be required as much then queries will become simpler. @aarshkshah1992 please share your views as well before I get into to the changes.
As for the --older-than flag, it makes much more sense than from, but the other prune command prune-msgindex was using the from flag so I wanted to maintain consistency.
- So should I update there (prune-msgindex) as well?
- Also I was thinking we can have separate pruning commands for each index, what are your thoughts on it @rvagg ?

rvagg · 2024-08-16T06:21:03Z

I don't think it need symmetry with the backfill commands, and honestly I find 'from' a little bit awkward for those anyway (I added that "going backwards" in there because of that awkwardness, I kind of wish they were something like --start=4100000 --end=-10000; but it's not too important I think.

Separate sub-commands makes sense to me rather than all in one. You may only have one of these indexes enabled anyway.

aarshkshah1992 · 2024-08-16T06:46:16Z

@akaladarshi I agree with Rodd. Looking at this now, separate pruning cmds for all Indices where we "prune everything older than epoch X" makes a lot of sense.

akaladarshi · 2024-08-16T07:49:48Z

@akaladarshi I agree with Rodd. Looking at this now, separate pruning cmds for all Indices where we "prune everything older than epoch X" makes a lot of sense.

I was inclined more towards having a single command to prune all the indexes at once and having individual commands as well.

From a code perspective, there won't be much change as I have already separated each index pruning into its function. It's a matter of calling those functions (individually or together).

rvagg · 2024-08-19T04:47:11Z

cmd/lotus-shed/indexes.go

+		basePath, err := homedir.Expand(cctx.String("repo"))
+		if err != nil {
+			return err
+		}
+
+		startHeight := cctx.Int64("older-than")
+		if startHeight == 0 {
+			currTs, err := api.ChainHead(ctx)
+			if err != nil {
+				return err
+			}
+
+			startHeight += int64(currTs.Height())
+
+			if startHeight == 0 {
+				return xerrors.Errorf("bogus start height %d", startHeight)
+			}
+		}


how about you refactor this into a utility function since this block is repeated 4 times: basePath, startHeight, err := parsePruneArgs(cctx)

rvagg · 2024-08-19T04:53:09Z

This is neat, thanks @akaladarshi. I have a suggestion, but I don't want to impose this as a requirement, just if you think it's interesting and worth doing: a --dry-run for each of these that does a query instead of a delete for the given args. You could SELECT count(*) FROM events WHERE height < ? and the same for >= ? so you can report something like "prune would delete X and leave Y events".

I think this should even work to do both in one query, for each table:

SELECT
    SUM(CASE WHEN height < ? THEN 1 ELSE 0 END) AS delete_count,
    SUM(CASE WHEN height >= ? THEN 1 ELSE 0 END) AS leave_count
FROM events

This is just a nice feature, I think though running this on my own node and I don't think I want to be pruning at the moment (I'm building up a big db which is helpful for debugging) but I would like to see what it would do, so that's what I came up with.

akaladarshi · 2024-08-19T14:30:15Z

This is neat, thanks @akaladarshi. I have a suggestion, but I don't want to impose this as a requirement, just if you think it's interesting and worth doing: a --dry-run for each of these that does a query instead of a delete for the given args. You could SELECT count(*) FROM events WHERE height < ? and the same for >= ? so you can report something like "prune would delete X and leave Y events".

I think this should even work to do both in one query, for each table:
SELECT
    SUM(CASE WHEN height < ? THEN 1 ELSE 0 END) AS delete_count,
    SUM(CASE WHEN height >= ? THEN 1 ELSE 0 END) AS leave_count
FROM events
This is just a nice feature, I think though running this on my own node and I don't think I want to be pruning at the moment (I'm building up a big db which is helpful for debugging) but I would like to see what it would do, so that's what I came up with.

That sounds like a good-to-have feature, I will start working on it once I am done with testing this PR locally with the calibration testnet node before merging and migrating of indexes to common DB.

akaladarshi · 2024-08-21T15:03:55Z

cmd/lotus-shed/indexes.go

+			blkMsgs, err := api.ChainGetBlockMessages(ctx, blockheader.Cid())
+			if err != nil {
+				log.Infof("failed to get block messages at height: %d", currTs.Height())
+				continue // should we break here?


@rvagg should we break here or keep going till the first tipset?

Is there a possibility, that a block is missing in between?

While testing I saw one block was missing, after that, it kept going on like this.

Testing logs:
`2024-08-21T20:21:39.118+0530 INFO lotus-shed lotus-shed/indexes.go:1148 failed to get block messages at height: 1867646

2024-08-21T20:21:39.122+0530 INFO lotus-shed lotus-shed/indexes.go:1148 failed to get block messages at height: 1867641

2024-08-21T20:21:39.127+0530 INFO lotus-shed lotus-shed/indexes.go:1148 failed to get block messages at height: 1867633

2024-08-21T20:21:39.134+0530 INFO lotus-shed lotus-shed/indexes.go:1148 failed to get block messages at height: 1867623`

Oh, this one is tricky! because we don't have an epoch to delete against we can't do it bulk, as you've discovered, but we also can't do it this way because we kind of want the inverse: we want to delete entries for messages where we don't have the message in the blockstore ourselves. So it's almost like: collect all messages, then delete everything not in that list. Which is impractical because that could get very large. The other problem with using the chain to do this is that there are going to be messages not on the canonical chain that we need to consider (i.e. there's been a reorg, so walking back from some tip doesn't lead us to tipsets that were previously considered canonical but are no longer), perhaps we want to include them, perhaps we should delete them because they are non-canonical, it's actually not even clear what you'd want to do because it depends on how the lookup API is being used. But conceptually what you'd want is something like: nothing in this txhashdb that isn't also in my blockstore. Because if I can't get to the original message anyway then there's no real help in being able to translate it from an eth hash.

BUT; just doing a quick look at the db, it's the one that we already have GC in already, and we achieve that by timestamp. If you turn on EthTxHashMappingLifetimeDays then it'll clean up according to the timestamp it was inserted at.

So I think the strategy needs to change for this table: we just can't do it on epoch unfortunately; we either need to not offer it here at all, or we need to come up with a timestamp and run something like the existing functionality:

lotus/chain/ethhashlookup/eth_transaction_hash_lookup.go

Line 131 in dbef5de

func (ei *EthTxHashLookup) DeleteEntriesOlderThan(days int) (int64, error) {

We can get a timestamp. So, if you want to keep this pruning here, you could keep the existing api.ChainGetTipSetByHeight(ctx, abi.ChainEpoch(startHeight), types.EmptyTSK) to get the tipset, look at the first block and get its timestamp (e.g. like this and then work out how many seconds ago it is and then you can run the delete using datetime() maths. Something like DELETE FROM eth_tx_hashes WHERE insertion_time < datetime('now', '-X seconds'). Note how sqlite datetime maths works:

sqlite> select datetime('now'), datetime('now', '-10 seconds'), datetime('now', '-86400 seconds'); 2024-08-22 03:38:03|2024-08-22 03:37:53|2024-08-21 03:38:03

This is one area where we could apply some reuse of existing code: in eth_transaction_hash_lookup.go, we could expose a method to reuse the existing delete statement, and since we only run this once per hour on the existing GC we can get rid of the preparedstatement too and expose a public function to run it on any instance of a db, here's a quick go at what what may look like over in that file:

func (ei *EthTxHashLookup) DeleteEntriesOlderThan(days int) (int64, error) { if ei.db == nil { return 0, xerrors.New("db closed") } epochs := abi.ChainEpoch(build.BlockDelaySecs * builtin.SecondsInDay * uint64(days)) return DeleteEntriesOlderThan(ei.db, epochs) } // DeleteEntriesOlderThan deletes entries older than the given number of epochs from now. func DeleteEntriesOlderThan(db *sql.DB, epochs abi.ChainEpoch) (int64, error) { secondsAgo := int64(epochs) * int64(build.BlockDelaySecs) res, err := db.Exec(deleteOlderThan, "-"+strconv.FormatInt(secondsAgo, 10)+" seconds") if err != nil { return 0, err } return res.RowsAffected() }

Updated as per your suggestion of reusing code.

akaladarshi · 2024-08-21T15:49:32Z

cmd/lotus-shed/indexes.go

+	totalRowsAffected += rowsAffected
+	log.Infof("pruned %s table, rows affected: %d", eventTable, rowsAffected)
+
+	res, err = tx.Exec(deleteEventEntriesByEventIDs, startHeight)


After executing deleteEventEntriesByEventIDs, res.RowsAffected() is returning 0 even though in DB data is being deleted.

My guess is that SQLite not able to figure out how many rows are affected because there is a subquery inside it.

What are your thoughts on this @rvagg.

you just need to swap this with deleteEventFromStartHeight - this query relies on the entries being in the event table because of the sub-select, but you've just deleted the ones that it's trying to match; so clean up event_entry first, then event and you're all good

As I mentioned the data is still being deleted but, it's not returning the number of rows affected.

Anyway, I will update it as you mentioned.

But how do you know it's being deleted? you've lost the entried they join with on the event table, have you tried a select count(*) from event and select count(*) from event_index to see that they're actually deleted? Because of the subquery working on rows that were previously deleted, you should not have the join and therefore not get deletion.

You're correct, I was checking it the wrong way.

rvagg · 2024-08-22T03:08:00Z

cmd/lotus-shed/indexes.go

-		defer closer()
-		ctx := lcli.ReqContext(cctx)
+		api := srvc.FullNodeAPI()
+		closer := srvc.Close


maybe just keep the pattern the same across these, you have some churn in naming and you have an extra closer here; the one below in pruneTxHashCmd is clean:

srv, err := lcli.GetFullNodeServices(cctx) if err != nil { return err } defer srv.Close() //nolint:errcheck api := srv.FullNodeAPI() ctx := lcli.ReqContext(cctx)

rvagg · 2024-08-22T03:09:39Z

cmd/lotus-shed/indexes.go

+}
+
+// pruneEventsIndex is a helper function that prunes the events index.db for a number of epochs starting from a specified height
+func pruneEventsIndex(_ context.Context, basePath string, startHeight int64) error {


just ditch the context arg if you have no way of using it, I know it's nice to have symmetry but if you don't need the symmetry for any tactical reason then just keep it clean

rvagg · 2024-08-22T03:11:47Z

cmd/lotus-shed/indexes.go

+
+// database related constants
+const (
+	databaseType   = "sqlite"


I found this naming confusing when parsing the path resolution below, perhaps something like indexesDir? i.e. it's the name of a directory and isn't used to refer to the type of database anywhere

I didn't realise this until after I ran the node. Initially, I thought it was DB-type.

Anyway, I have updated it.

rvagg · 2024-08-22T03:14:13Z

cmd/lotus-shed/indexes.go

+		err := db.Close()
+		if err != nil {
+			fmt.Printf("ERROR: closing db: %s", err)
+		}


prefer compact form where you can get away with it, IMO it helps with Go's nasty eye-sprawl

Suggested change

err := db.Close()

if err != nil {

fmt.Printf("ERROR: closing db: %s", err)

}

if err := db.Close(); err != nil {

fmt.Printf("ERROR: closing db: %s", err)

}

rvagg · 2024-08-22T03:17:42Z

cmd/lotus-shed/indexes.go

+	}
+
+	totalRowsAffected += rowsAffected
+	log.Infof("pruned %s table, rows affected: %d", eventTable, rowsAffected)


not critical: I suggest keeping track of these numbers separately and then logging after you've committed the transaction, because until you do, they're not really gone; if you have a rollback then these log messages will be incorrect

if you're concerned about progress logging, print a message before each execution: log.Infof("pruning `%s` table", eventTable); then at least the user knows its working if this takes a long time for some reason (it shouldn't unless it's on a really slow disk)

rvagg · 2024-08-22T03:19:42Z

cmd/lotus-shed/indexes.go

+	err = tx.Commit()
+	if err != nil {
+		return xerrors.Errorf("error committing tx: %w", err)
+	}


Suggested change

err = tx.Commit()

if err != nil {

return xerrors.Errorf("error committing tx: %w", err)

}

if err = tx.Commit(); err != nil {

return xerrors.Errorf("error committing tx: %w", err)

}

rvagg · 2024-08-22T03:21:03Z

cmd/lotus-shed/indexes.go

+		err := db.Close()
+		if err != nil {
+			fmt.Printf("ERROR: closing db: %s", err)
+		}


Suggested change

err := db.Close()

if err != nil {

fmt.Printf("ERROR: closing db: %s", err)

}

if err := db.Close(); err != nil {

fmt.Printf("ERROR: closing db: %s", err)

}

BigLep · 2024-09-17T00:25:00Z

@akaladarshi : what are the next steps for this PR so it can be merged?

akaladarshi · 2024-09-18T05:59:20Z

@akaladarshi : what are the next steps for this PR so it can be merged?

@BigLep I am not sure about this PR as of now because we have removed the existing indexes in favour of #12421.

I think @aarshkshah1992 or @rvagg are the best people who can comment on this.

feat: add prune all indexes command

b7dd1e3

akaladarshi marked this pull request as ready for review August 15, 2024 13:55

rvagg reviewed Aug 16, 2024

View reviewed changes

feat: remove epochs and update delete queries

a11ac44

akaladarshi changed the title ~~feat(index): add prune all indexes command~~ feat(index): add lotus shed commands to prune all indexes Aug 16, 2024

akaladarshi added 2 commits August 16, 2024 18:52

update changelog

fe483b3

fix: ci issues

88f04cc

akaladarshi requested a review from rvagg August 19, 2024 04:17

rvagg reviewed Aug 19, 2024

View reviewed changes

akaladarshi added 2 commits August 19, 2024 19:09

Merge branch 'master' into akaladarshi/lotus-shed-prune-all-indexes-cmd

d3be29d

address comments

67817cc

akaladarshi requested a review from rvagg August 20, 2024 04:20

fix: ci issues

fb1758e

akaladarshi commented Aug 21, 2024

View reviewed changes

rvagg reviewed Aug 22, 2024

View reviewed changes

akaladarshi added 2 commits August 22, 2024 17:16

Merge branch 'master' into akaladarshi/lotus-shed-prune-all-indexes-cmd

a2a46fa

address comments

2f2a23f

akaladarshi requested a review from rvagg August 26, 2024 06:24

akaladarshi force-pushed the akaladarshi/lotus-shed-prune-all-indexes-cmd branch from e60187d to d092ede Compare August 26, 2024 06:37

fix: ci issues

e25ebee

akaladarshi force-pushed the akaladarshi/lotus-shed-prune-all-indexes-cmd branch from d092ede to e25ebee Compare August 26, 2024 06:37

akaladarshi added 3 commits August 26, 2024 13:07

fix: ci issue

2d29ff4

small change

cb1c30f

feat: use days for pruning tx db

3519664

BigLep assigned akaladarshi Sep 17, 2024

feat(index): add lotus shed commands to prune all indexes #12393

Are you sure you want to change the base?

feat(index): add lotus shed commands to prune all indexes #12393

Conversation

akaladarshi commented Aug 15, 2024 • edited Loading

Related Issues

Proposed Changes

akaladarshi commented Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Aug 16, 2024

akaladarshi commented Aug 16, 2024 • edited Loading

rvagg commented Aug 16, 2024

aarshkshah1992 commented Aug 16, 2024

akaladarshi commented Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Aug 19, 2024

akaladarshi commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akaladarshi Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigLep commented Sep 17, 2024

akaladarshi commented Sep 18, 2024

akaladarshi commented Aug 15, 2024 •

edited

Loading

akaladarshi commented Aug 15, 2024 •

edited

Loading

akaladarshi commented Aug 16, 2024 •

edited

Loading

akaladarshi commented Aug 16, 2024 •

edited

Loading

akaladarshi Aug 21, 2024 •

edited

Loading

rvagg Aug 22, 2024 •

edited

Loading