Add ConsensusV2 object support to `sui-indexer{,alt}` #20486

aschran · 2024-12-02T20:11:04Z

Updates sum_coin_balances and wal_coin_balances tables to add a column for coin_owner_kind, so that balances can be tracked separately for fastpath vs. consensus coins.

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.

Updates `sum_coin_balances` and `wal_coin_balances` tables to add a column for `owner_kind`, so that balances can be tracked separately for address-owned vs. consensus coins.

vercel · 2024-12-02T20:11:08Z

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	Dec 5, 2024 4:32pm

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Dec 5, 2024 4:32pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Dec 5, 2024 4:32pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Dec 5, 2024 4:32pm

amnn

Based on what I understood of the plan, we want to treat objects and coins slightly differently when it comes to consensus-management (in that we want to abstract it away for objects, and not for coins), which will require some minor changes, but the sketch is there, thanks @aschran !

crates/sui-indexer-alt/migrations/2024-10-28-144002_sum_coin_balances/up.sql

amnn · 2024-12-03T14:23:02Z

crates/sui-indexer-alt/migrations/2024-12-02-185822_add_coin_balance_owner_kind/up.sql

@@ -0,0 +1,6 @@
+ALTER TABLE sum_coin_balances
+ADD COLUMN owner_kind SMALLINT NOT NULL DEFAULT 1;


Once we have switched over to a version of the indexer that knows about this column, we should come back and drop the DEFAULT here.

Also, we are going to need to update all the indices on all these tables -- wherever they mentioned owner_id we now need to prefix it with an owner_kind.

added index updates

amnn · 2024-12-03T14:26:30Z

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

@@ -99,8 +99,7 @@ impl Processor for SumObjTypes {
                                    Owner::ObjectOwner(_) => StoredOwnerKind::Object,
                                    Owner::Shared { .. } => StoredOwnerKind::Shared,
                                    Owner::Immutable => StoredOwnerKind::Immutable,
-                                    // TODO: Implement support for ConsensusV2 objects.
-                                    Owner::ConsensusV2 { .. } => todo!(),
+                                    Owner::ConsensusV2 { .. } => StoredOwnerKind::ConsensusV2,


I thought that for objects we wanted to treat consensus-owned objects as address-owned if they the authenticator maps to a single address, which means no new kind here. (Then there was a future question of how we handle party objects which we kind of punted on, but the options varied from treating them like shared objects, to having some separate category for jointly-owned objects).

updated as per huddle

amnn · 2024-12-03T14:29:51Z

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

+                    // ConsensusV2 objects are treated as address-owned for now in indexers.
+                    // This will need to be updated if additional Authenticators are added.
+                    Owner::ConsensusV2 { authenticator, .. } => (
+                        StoredOwnerKind::ConsensusV2,


We should use a different enum here to the one we use for objects in general, because while we don't want to distinguish consensus-managed single-owned objects from address-owned objects, we do want to do that for coins.

created a new enum

amnn · 2024-12-03T14:33:11Z

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

+                    // This will need to be updated if additional Authenticators are added.
+                    Owner::ConsensusV2 { authenticator, .. } => (
+                        StoredOwnerKind::ConsensusV2,
+                        authenticator.as_single_owner(),


Just noting my momentary confusion that this doesn't return an Option<...>!

yes, i put a comment on the function to clarify in another PR

lxfind · 2024-12-03T19:18:14Z

There is now also a obj_info pipeline

amnn

This change now looks good to me! cc @lxfind though as he is currently working on the sum_coin_balances and sum_obj_types pipelines (or their equivalents), so we may need to incorporate changes there.

I've also left a comment on how to make the index changes go through smoothly. Once those changes are in, we can coordinate the land and proactively running the migration so everything keeps working smoothly.

amnn · 2024-12-05T13:48:18Z

crates/sui-indexer-alt/migrations/2024-12-02-185822_add_coin_balance_owner_kind/up.sql

+DROP INDEX sum_coin_balances_owner_type;
+CREATE INDEX sum_coin_balances_owner_type


I'm thinking of some ways to make this easier to land without causing the indexer to hang when it starts, and I think I have an idea:

We can get postgres to drop and create these indices concurrently, which will mean they won't hang other things that are going on at the same time (this will allow us to run the migrations proactively while the indexer is running),

and then turn them into IF EXISTS/IF NOT EXISTS style statements so that they become idempotent, (which means after we've run it proactively, when the migration runs, it knows not to drop and regenerate the indices),

and finally, we need to give the new index a new name so that IF EXISTS/IF NOT EXISTS would recognise it as different from the old index.

The statements for creating and dropping the indices would look like this then:

Suggested change

DROP INDEX sum_coin_balances_owner_type;

CREATE INDEX sum_coin_balances_owner_type

DROP INDEX CONCURRENTLY IF EXISTS sum_coin_balances_owner_type;

CREATE INDEX CONCURRENTLY IF NOT EXISTS sum_coin_balances_owner_kind_owner_id_type;

But you will also need to split it up over separate migration scripts (because each script is its own transaction and postgres won't like it if you combine non-blocking concurrent operations with other work in a transaction). I've had to do this once before in #19543 so that can serve as a template (there's an extra TOML file that needs to go in each migration folder to tell diesel to not batch up migrations into a transaction as well).

(ditto for the wal tables below).

EDIT: tweaked index name.

I did this, let me know if it looks correct. when I run generate_schema.sh locally it fails with an error

2024-12-05 10:37:44.702 EST [78369] ERROR: DROP INDEX CONCURRENTLY cannot run inside a transaction block

But I have the metadata.toml files there as you suggested - is anything else missing?

The issue is that the migrations need to be split up even further -- each DROP INDEX and CREATE INDEX needs to be in their own script (because they can't mingle with each other either).

Thanks, done

amnn

LGTM! @aschran, once you have landed this, please ping the Data Platform slack channel, so someone (probably myself or @lxfind) can run the migrations proactively, so we can avoid the next deploy hanging indefinitely.

## Description Partial revert of #20486 (only the changes relevant to `sui-indexer-alt`), as the migrations associated with that change are taking too long to run on the production database. We will have to revisit how to set-up those migrations so that they run in a reasonable time. ## Test plan Local run of `sui-indexer-alt` + CI.

…20546) ## Description Partial revert of #20486 (only the changes relevant to `sui-indexer-alt`), as the migrations associated with that change are taking too long to run on the production database. We will have to revisit how to set-up those migrations so that they run in a reasonable time. ## Test plan Local run of `sui-indexer-alt` + CI. --- ## 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. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:

Add ConsensusV2 object support to sui-indexer{,alt}

363a126

Updates `sum_coin_balances` and `wal_coin_balances` tables to add a column for `owner_kind`, so that balances can be tracked separately for address-owned vs. consensus coins.

aschran requested a review from amnn December 2, 2024 20:11

aschran temporarily deployed to sui-typescript-aws-kms-test-env December 2, 2024 20:11 — with GitHub Actions Inactive

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

amnn reviewed Dec 3, 2024

View reviewed changes

amnn requested review from lxfind, bmwill, emmazzz, gegaowp and wlmyng December 3, 2024 14:34

map consensusv2 objects onto existing address owner_kind

2e3e7b7

aschran requested a review from amnn December 4, 2024 21:11

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

amnn reviewed Dec 5, 2024

View reviewed changes

Merge branch 'main' into aschran/cov2-indexers

5d242dd

aschran temporarily deployed to sui-typescript-aws-kms-test-env December 5, 2024 15:17 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-kiosk December 5, 2024 15:18 View deployment

vercel bot deployed to Preview – sui-typescript-docs December 5, 2024 15:18 View deployment

vercel bot deployed to Preview – multisig-toolkit December 5, 2024 15:18 View deployment

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

add clarifying comment to obj_info table

1fe567a

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

typo fix

030b6c4

aschran temporarily deployed to sui-typescript-aws-kms-test-env December 5, 2024 15:20 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 5, 2024 15:22 View deployment

separate index changes into a different migration

c777f43

aschran temporarily deployed to sui-typescript-aws-kms-test-env December 5, 2024 15:38 — with GitHub Actions Inactive

aschran requested a review from amnn December 5, 2024 15:39

vercel bot deployed to Preview – sui-docs December 5, 2024 15:39 View deployment

separate migrations even further

e0a5944

aschran temporarily deployed to sui-typescript-aws-kms-test-env December 5, 2024 16:30 — with GitHub Actions Inactive

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

amnn approved these changes Dec 6, 2024

View reviewed changes

aschran merged commit b44f391 into main Dec 6, 2024
52 checks passed

aschran deleted the aschran/cov2-indexers branch December 6, 2024 15:18

amnn mentioned this pull request Dec 8, 2024

revert(#20486): Add ConsensusV2 object support to sui-indexer-alt #20546

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ConsensusV2 object support to `sui-indexer{,alt}` #20486

Add ConsensusV2 object support to `sui-indexer{,alt}` #20486

aschran commented Dec 2, 2024 •

edited

Loading

vercel bot commented Dec 2, 2024 •

edited

Loading

amnn left a comment

amnn Dec 3, 2024

amnn Dec 3, 2024

aschran Dec 4, 2024

amnn Dec 3, 2024

aschran Dec 4, 2024

amnn Dec 3, 2024

aschran Dec 4, 2024

amnn Dec 3, 2024

aschran Dec 4, 2024

lxfind commented Dec 3, 2024

amnn left a comment

amnn Dec 5, 2024 •

edited

Loading

aschran Dec 5, 2024

amnn Dec 5, 2024

aschran Dec 5, 2024

amnn left a comment

		@@ -0,0 +1,6 @@
		ALTER TABLE sum_coin_balances
		ADD COLUMN owner_kind SMALLINT NOT NULL DEFAULT 1;

		DROP INDEX sum_coin_balances_owner_type;
		CREATE INDEX sum_coin_balances_owner_type

Add ConsensusV2 object support to sui-indexer{,alt} #20486

Add ConsensusV2 object support to sui-indexer{,alt} #20486

Conversation

aschran commented Dec 2, 2024 • edited Loading

Release notes

vercel bot commented Dec 2, 2024 • edited Loading

amnn left a 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

lxfind commented Dec 3, 2024

amnn left a comment

Choose a reason for hiding this comment

amnn Dec 5, 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

amnn left a comment

Choose a reason for hiding this comment

Add ConsensusV2 object support to `sui-indexer{,alt}` #20486

Add ConsensusV2 object support to `sui-indexer{,alt}` #20486

aschran commented Dec 2, 2024 •

edited

Loading

vercel bot commented Dec 2, 2024 •

edited

Loading

amnn Dec 5, 2024 •

edited

Loading