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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Dec 6, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
363a126
Add ConsensusV2 object support to `sui-indexer{,alt}`
aschran Dec 2, 2024
2e3e7b7
map consensusv2 objects onto existing address owner_kind
aschran Dec 4, 2024
5d242dd
Merge branch 'main' into aschran/cov2-indexers
aschran Dec 5, 2024
1fe567a
add clarifying comment to obj_info table
aschran Dec 5, 2024
030b6c4
typo fix
aschran Dec 5, 2024
c777f43
separate index changes into a different migration
aschran Dec 5, 2024
e0a5944
separate migrations even further
aschran Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
map consensusv2 objects onto existing address owner_kind
  • Loading branch information
@aschran
aschran committed Dec 4, 2024
commit 2e3e7b79ffeaafb7d8326b3317971c51b9076c4c
7 changes: 5 additions & 2 deletions crates/sui-indexer-alt/migrations/2024-10-27-150938_sum_obj_types/up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ CREATE TABLE IF NOT EXISTS sum_obj_types
(
object_id BYTEA PRIMARY KEY,
object_version BIGINT NOT NULL,
-- TODO-DNS Question for reviewers: what do I do about updating
-- now-invalid comments on existing columns, like below? Just change here?
-- An enum describing the object's ownership model:
--
-- Immutable = 0,
Expand All @@ -19,6 +17,11 @@ CREATE TABLE IF NOT EXISTS sum_obj_types
-- another object (kind 2), which relates to dynamic fields, and an object
-- that is owned by another object's address (kind 1), which relates to
-- transfer-to-object.
--
-- Warning: This column may look similar to the concept of "ObjectOwner"
-- but is NOT the same. For purposes of determining owner_kind, ConsensusV2
-- objects mapped onto the variants described above based on their
-- authenticator.
owner_kind SMALLINT NOT NULL,
-- The address for address-owned objects, and the parent object for
-- object-owned objects.
Expand Down
5 changes: 1 addition & 4 deletions crates/sui-indexer-alt/migrations/2024-10-28-144002_sum_coin_balances/up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,7 @@ CREATE TABLE IF NOT EXISTS sum_coin_balances
(
object_id BYTEA PRIMARY KEY,
object_version BIGINT NOT NULL,
-- TODO-DNS Question for reviewers: what do I do about updating
-- now-invalid comments on existing columns, like below? Just change here?
-- The address that owns this version of the coin (it is guaranteed to be
-- address-owned).
-- The address that owns this version of the coin.
owner_id BYTEA NOT NULL,
-- The type of the coin, as a BCS-serialized `TypeTag`. This is only the
-- marker type, and not the full object type (e.g. `0x0...02::sui::SUI`).
Expand Down
12 changes: 10 additions & 2 deletions crates/sui-indexer-alt/migrations/2024-12-02-185822_add_coin_balance_owner_kind/up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
ALTER TABLE sum_coin_balances
ADD COLUMN owner_kind SMALLINT NOT NULL DEFAULT 1;
ADD COLUMN coin_owner_kind SMALLINT NOT NULL DEFAULT 1;

DROP INDEX sum_coin_balances_owner_type;
CREATE INDEX sum_coin_balances_owner_type
Copy link
Member

@amnn amnn Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done

ON sum_coin_balances (coin_owner_kind, owner_id, coin_type, coin_balance, object_id, object_version);

ALTER TABLE wal_coin_balances
ADD COLUMN owner_kind SMALLINT;
ADD COLUMN coin_owner_kind SMALLINT;
UPDATE wal_coin_balances SET owner_kind = 1 WHERE owner_id IS NOT NULL;

DROP INDEX wal_coin_balances_owner_type;
CREATE INDEX wal_coin_balances_owner_type
ON wal_coin_balances (coin_owner_kind, owner_id, coin_type, coin_balance, object_id, object_version);
12 changes: 7 additions & 5 deletions crates/sui-indexer-alt/src/handlers/sum_coin_balances.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use sui_types::{

use crate::{
db,
models::objects::{StoredObjectUpdate, StoredOwnerKind, StoredSumCoinBalance},
models::objects::{StoredCoinOwnerKind, StoredObjectUpdate, StoredSumCoinBalance},
pipeline::{sequential::Handler, Processor},
schema::sum_coin_balances,
};
Expand Down Expand Up @@ -100,12 +100,12 @@ impl Processor for SumCoinBalances {
};

// Coin balance only tracks address-owned or ConsensusV2 objects
let (owner_kind, owner_id) = match object.owner() {
Owner::AddressOwner(owner_id) => (StoredOwnerKind::Address, owner_id),
let (coin_owner_kind, owner_id) = match object.owner() {
Owner::AddressOwner(owner_id) => (StoredCoinOwnerKind::Fastpath, owner_id),
// 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,
StoredCoinOwnerKind::Consensus,
authenticator.as_single_owner(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

),

Expand All @@ -132,7 +132,7 @@ impl Processor for SumCoinBalances {
owner_id: owner_id.to_vec(),
coin_type: coin_type.clone(),
coin_balance: coin.balance.value() as i64,
owner_kind,
coin_owner_kind,
}),
});
}
Expand Down Expand Up @@ -183,6 +183,8 @@ impl Handler for SumCoinBalances {
sum_coin_balances::owner_id.eq(excluded(sum_coin_balances::owner_id)),
sum_coin_balances::coin_balance
.eq(excluded(sum_coin_balances::coin_balance)),
sum_coin_balances::coin_owner_kind
.eq(excluded(sum_coin_balances::coin_owner_kind)),
))
.execute(conn),
),
Expand Down
4 changes: 3 additions & 1 deletion crates/sui-indexer-alt/src/handlers/sum_obj_types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,9 @@ impl Processor for SumObjTypes {
Owner::ObjectOwner(_) => StoredOwnerKind::Object,
Owner::Shared { .. } => StoredOwnerKind::Shared,
Owner::Immutable => StoredOwnerKind::Immutable,
Owner::ConsensusV2 { .. } => StoredOwnerKind::ConsensusV2,
// ConsensusV2 objects are treated as address-owned for now in indexers.
// This will need to be updated if additional Authenticators are added.
Owner::ConsensusV2 { .. } => StoredOwnerKind::Address,
},

owner_id: match object.owner() {
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-indexer-alt/src/handlers/wal_coin_balances.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl Handler for WalCoinBalances {

cp_sequence_number: value.cp_sequence_number as i64,

owner_kind: value.update.as_ref().map(|o| o.owner_kind),
coin_owner_kind: value.update.as_ref().map(|o| o.coin_owner_kind),
})
.collect();

Expand Down
39 changes: 35 additions & 4 deletions crates/sui-indexer-alt/src/models/objects.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,14 @@ pub enum StoredOwnerKind {
Address = 1,
Object = 2,
Shared = 3,
ConsensusV2 = 4,
}

#[derive(AsExpression, FromSqlRow, Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
#[diesel(sql_type = SmallInt)]
#[repr(i16)]
pub enum StoredCoinOwnerKind {
Fastpath = 0,
Consensus = 1,
}

#[derive(Insertable, Debug, Clone, FieldCount)]
Expand All @@ -59,7 +66,7 @@ pub struct StoredSumCoinBalance {
pub owner_id: Vec<u8>,
pub coin_type: Vec<u8>,
pub coin_balance: i64,
pub owner_kind: StoredOwnerKind,
pub coin_owner_kind: StoredCoinOwnerKind,
}

#[derive(Insertable, Debug, Clone, FieldCount)]
Expand All @@ -84,7 +91,7 @@ pub struct StoredWalCoinBalance {
pub coin_type: Option<Vec<u8>>,
pub coin_balance: Option<i64>,
pub cp_sequence_number: i64,
pub owner_kind: Option<StoredOwnerKind>,
pub coin_owner_kind: Option<StoredCoinOwnerKind>,
}

#[derive(Insertable, Debug, Clone)]
Expand All @@ -111,7 +118,6 @@ where
StoredOwnerKind::Address => 1.to_sql(out),
StoredOwnerKind::Object => 2.to_sql(out),
StoredOwnerKind::Shared => 3.to_sql(out),
StoredOwnerKind::ConsensusV2 => 4.to_sql(out),
}
}
}
Expand All @@ -130,3 +136,28 @@ where
})
}
}

impl<DB: Backend> serialize::ToSql<SmallInt, DB> for StoredCoinOwnerKind
where
i16: serialize::ToSql<SmallInt, DB>,
{
fn to_sql<'b>(&'b self, out: &mut serialize::Output<'b, '_, DB>) -> serialize::Result {
match self {
StoredCoinOwnerKind::Fastpath => 0.to_sql(out),
StoredCoinOwnerKind::Consensus => 1.to_sql(out),
}
}
}

impl<DB: Backend> deserialize::FromSql<SmallInt, DB> for StoredCoinOwnerKind
where
i16: deserialize::FromSql<SmallInt, DB>,
{
fn from_sql(raw: DB::RawValue<'_>) -> deserialize::Result<Self> {
Ok(match i16::from_sql(raw)? {
0 => StoredCoinOwnerKind::Fastpath,
1 => StoredCoinOwnerKind::Consensus,
o => return Err(format!("Unexpected StoredCoinOwnerKind: {o}").into()),
})
}
}
4 changes: 2 additions & 2 deletions crates/sui-indexer-alt/src/schema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ diesel::table! {
owner_id -> Bytea,
coin_type -> Bytea,
coin_balance -> Int8,
owner_kind -> Int2,
coin_owner_kind -> Int2,
}
}

Expand Down Expand Up @@ -210,7 +210,7 @@ diesel::table! {
coin_type -> Nullable<Bytea>,
coin_balance -> Nullable<Int8>,
cp_sequence_number -> Int8,
owner_kind -> Nullable<Int2>,
coin_owner_kind -> Nullable<Int2>,
}
}

Expand Down
8 changes: 3 additions & 5 deletions crates/sui-indexer/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,6 @@ pub enum OwnerType {
Address = 1,
Object = 2,
Shared = 3,
ConsensusV2 = 4,
}

pub enum ObjectStatus {
Expand Down Expand Up @@ -257,10 +256,9 @@ pub fn owner_to_owner_info(owner: &Owner) -> (OwnerType, Option<SuiAddress>) {
Owner::Immutable => (OwnerType::Immutable, None),
// ConsensusV2 objects are treated as singly-owned for now in indexers.
// This will need to be updated if additional Authenticators are added.
Owner::ConsensusV2 { authenticator, .. } => (
OwnerType::ConsensusV2,
Some(*authenticator.as_single_owner()),
),
Owner::ConsensusV2 { authenticator, .. } => {
(OwnerType::Address, Some(*authenticator.as_single_owner()))
}
}
}

Expand Down
8 changes: 3 additions & 5 deletions crates/sui-mvr-indexer/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,6 @@ pub enum OwnerType {
Address = 1,
Object = 2,
Shared = 3,
ConsensusV2 = 4,
}

pub enum ObjectStatus {
Expand Down Expand Up @@ -257,10 +256,9 @@ pub fn owner_to_owner_info(owner: &Owner) -> (OwnerType, Option<SuiAddress>) {
Owner::Immutable => (OwnerType::Immutable, None),
// ConsensusV2 objects are treated as singly-owned for now in indexers.
// This will need to be updated if additional Authenticators are added.
Owner::ConsensusV2 { authenticator, .. } => (
OwnerType::ConsensusV2,
Some(*authenticator.as_single_owner()),
),
Owner::ConsensusV2 { authenticator, .. } => {
(OwnerType::Address, Some(*authenticator.as_single_owner()))
}
}
}

Expand Down
Loading