[indexer-alt] add prune impls for each pipeline #20635

emmazzz · 2024-12-14T04:10:53Z

Description

Added prune implementations for pipeline inside indexer alt schema, built upon Will's cp mapping PR.

Test plan

Will add tests.

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

vercel · 2024-12-14T04:10:54Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 9, 2025 5:01pm

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Jan 9, 2025 5:01pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Jan 9, 2025 5:01pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Jan 9, 2025 5:01pm

emmazzz · 2024-12-14T08:03:50Z

crates/sui-indexer-alt/src/handlers/kv_epoch_starts.rs

+    async fn prune(range: PrunableRange, conn: &mut db::Connection<'_>) -> Result<usize> {
+        let (from, to) = range.containing_epochs();
+        let filter = kv_epoch_starts::table
+            .filter(kv_epoch_starts::epoch.between(from as i64, to as i64 - 1));


I have not fully convinced myself that pruning epoch grained tables will just work like this, even when our watermark and retention are checkpoint grained. I will add a test tomorrow to make sure.

Indeed -- and I would say that if this implementation doesn't behave correctly, it would be good to change the helper function in PrunableRange so that all prune impls can follow the same pattern (regardless of whether they are epoch-, checkpoint-, or transaction-grained), rather than change the epoch-grained impls to have a slightly different structure.

amnn

This looks great, thanks @emmazzz. There are some suggested changes on @wlmyng's PR that may affect this one (how the epoch helpers are implemented might introduce an off-by-one difference in this PR, and there is also a suggestion about changing the PrunableRange interface to move the responsibility to translate bounds into the individual prune impls).

I'll leave it with you and @wlmyng to coordinate those changes and then land both PRs!

amnn · 2024-12-15T16:01:40Z

crates/sui-indexer-alt/src/handlers/kv_transactions.rs

Given we can now delete by tx sequence number, should we get rid of the index on cp_sequence_number and write this prune impl based on the tx_interval?

Do we actually have an index on cp_sequence_number? Seems like it is a field on the table, but no corresponding index. We might as well proceed with implementation based on tx_interval

Ah, I must've missed something ... it looks like kv_transactions has only the cp_sequence_number field. So we'd need to backfill that. And since the primary key is on tx_digest, we'd need to introduce an index on tx_sequence_number

amnn · 2024-12-15T16:03:24Z

crates/sui-indexer-alt/src/handlers/kv_epoch_starts.rs

+    async fn prune(range: PrunableRange, conn: &mut db::Connection<'_>) -> Result<usize> {
+        let (from, to) = range.containing_epochs();
+        let filter = kv_epoch_starts::table
+            .filter(kv_epoch_starts::epoch.between(from as i64, to as i64 - 1));


Indeed -- and I would say that if this implementation doesn't behave correctly, it would be good to change the helper function in PrunableRange so that all prune impls can follow the same pattern (regardless of whether they are epoch-, checkpoint-, or transaction-grained), rather than change the epoch-grained impls to have a slightly different structure.

amnn

I think again this PR would change because of comments on the earlier PR, but I think those changes should be mechanical, so accepting to unblock!

crates/sui-indexer-alt/src/handlers/ev_emit_mod.rs

lxfind · 2025-01-07T16:06:26Z

crates/sui-indexer-alt/src/handlers/ev_emit_mod.rs

            .await?)
    }
+
+    async fn prune(from: u64, to: u64, conn: &mut db::Connection<'_>) -> Result<usize> {


Could you update the to to to_exclusive in all implementations?

lxfind · 2025-01-07T16:06:35Z

crates/sui-indexer-alt/src/handlers/ev_emit_mod.rs

+        } = tx_interval(conn, from..to).await?;
+
+        let filter = ev_emit_mod::table
+            .filter(ev_emit_mod::tx_sequence_number.between(from_tx as i64, to_tx as i64 - 1));


The to_tx is the first tx of the to_exclusive checkpoint and should not be pruned

amnn

Just some post-land questions/notes!

amnn · 2025-01-14T11:38:33Z

crates/sui-indexer-alt/src/handlers/kv_epoch_ends.rs

+            start: from_epoch,
+            end: to_epoch,
+        } = epoch_interval(conn, from..to_exclusive).await?;
+        if from_epoch < to_epoch {


Is this check strictly necessary? If they were equal, the between would contain conflicting constraints and nothing would be captured by it, right?

The reason I'm asking is that it piqued my interest that this impl had this check, but the previous ones didn't. If there was a correctness reason to have this check (i.e. that we will get some sort of error by having the bounds passed to BETWEEN inverted), then we are relying on that situation not coming up in practice because we produce checkpoints (and therefore transactions) more often than we prune -- that makes me nervous because we could definitely end up falling foul of this in test scenarios.

If there is not a correctness reason to have this test, then the only other reason I could think of was performance, but this is one of the cheapest tables we have.

I don't think Postgres has an issue with BETWEEN being inverted, but this was just something explicit to handle a possibility unique to epoch. Given [from, to) checkpoints, they may both fall under the same epoch. Conceptually we don't want to prune that epoch

pruning from chkpt 0 to 1 from_epoch: 0, to_epoch: 0 skipping because from_epoch >= to_epoch pruning from chkpt 1 to 2 from_epoch: 0, to_epoch: 1 pruning from 0 to 0 pruning from chkpt 2 to 3 from_epoch: 1, to_epoch: 1 skipping because from_epoch >= to_epoch pruning from chkpt 3 to 4 from_epoch: 1, to_epoch: 1 skipping because from_epoch >= to_epoch

If you do run a query ... delete between 0 and 0 - 1 Postgres returns no results from execution

amnn · 2025-01-14T11:39:45Z

crates/sui-indexer-alt/src/handlers/kv_transactions.rs

+        // TODO: use tx_interval. `tx_sequence_number` needs to be added to this table, and an index
+        // created as its primary key is on `tx_digest`.


No, let's keep this table as it is. Its schema needs to match the schema and indices need to match what is offered in the KV store as much as possible.

emmazzz temporarily deployed to sui-typescript-aws-kms-test-env December 14, 2024 04:11 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 14, 2024 04:12 View deployment

emmazzz marked this pull request as ready for review December 14, 2024 08:01

emmazzz temporarily deployed to sui-typescript-aws-kms-test-env December 14, 2024 08:01 — with GitHub Actions Inactive

emmazzz commented Dec 14, 2024

View reviewed changes

emmazzz requested review from amnn and wlmyng December 14, 2024 08:04

amnn reviewed Dec 15, 2024

View reviewed changes

wlmyng force-pushed the indexer-alt-cp-mapping-for-pruning branch from 160d0a8 to 6cabf8a Compare December 19, 2024 18:21

wlmyng requested a review from ronny-mysten as a code owner December 19, 2024 18:21

wlmyng force-pushed the indexer-alt-prune-impls branch from 2ce69cb to 438479b Compare December 19, 2024 18:25

wlmyng temporarily deployed to sui-typescript-aws-kms-test-env December 19, 2024 18:25 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 19, 2024 18:27 View deployment

wlmyng temporarily deployed to sui-typescript-aws-kms-test-env December 19, 2024 19:10 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 19, 2024 19:11 View deployment

wlmyng requested a review from amnn December 20, 2024 01:19

amnn approved these changes Dec 22, 2024

View reviewed changes

crates/sui-indexer-alt/src/handlers/ev_emit_mod.rs Outdated Show resolved Hide resolved

wlmyng force-pushed the indexer-alt-cp-mapping-for-pruning branch 3 times, most recently from 92c75e5 to 87504e7 Compare December 26, 2024 17:11

wlmyng force-pushed the indexer-alt-prune-impls branch from b07a86e to 4726c70 Compare December 26, 2024 17:12

wlmyng temporarily deployed to sui-typescript-aws-kms-test-env December 26, 2024 17:12 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 26, 2024 17:16 View deployment

wlmyng force-pushed the indexer-alt-prune-impls branch from 4726c70 to e8252bf Compare December 26, 2024 18:45

wlmyng temporarily deployed to sui-typescript-aws-kms-test-env December 26, 2024 18:45 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 26, 2024 18:47 View deployment

lxfind reviewed Jan 7, 2025

View reviewed changes

wlmyng force-pushed the indexer-alt-cp-mapping-for-pruning branch from 87504e7 to 4e0e03b Compare January 8, 2025 00:17

Base automatically changed from indexer-alt-cp-mapping-for-pruning to main January 8, 2025 00:54

wlmyng force-pushed the indexer-alt-prune-impls branch from e8252bf to 040df88 Compare January 8, 2025 00:54

wlmyng temporarily deployed to sui-typescript-aws-kms-test-env January 8, 2025 00:54 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 8, 2025 00:59 View deployment

wlmyng temporarily deployed to sui-typescript-aws-kms-test-env January 9, 2025 00:52 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 9, 2025 00:53 View deployment

lxfind approved these changes Jan 9, 2025

View reviewed changes

emmazzz and others added 3 commits January 9, 2025 08:55

[indexer-alt] add prune impls for each pipeline

2b36104

clean ups

efd9cf6

rustfmt

5ef7c04

wlmyng force-pushed the indexer-alt-prune-impls branch from 9fd872e to 5ef7c04 Compare January 9, 2025 16:56

wlmyng temporarily deployed to sui-typescript-aws-kms-test-env January 9, 2025 16:56 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 9, 2025 17:01 View deployment

wlmyng merged commit a114f51 into main Jan 9, 2025
50 of 51 checks passed

wlmyng deleted the indexer-alt-prune-impls branch January 9, 2025 19:17

amnn reviewed Jan 14, 2025

View reviewed changes

		// TODO: use tx_interval. `tx_sequence_number` needs to be added to this table, and an index
		// created as its primary key is on `tx_digest`.

[indexer-alt] add prune impls for each pipeline #20635

[indexer-alt] add prune impls for each pipeline #20635

Uh oh!

Conversation

emmazzz commented Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test plan

Release notes

Uh oh!

vercel bot commented Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emmazzz Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amnn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amnn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amnn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

emmazzz commented Dec 14, 2024 •

edited

Loading

vercel bot commented Dec 14, 2024 •

edited

Loading

emmazzz Dec 14, 2024 •

edited

Loading