[indexer-alt] add transaction indices pipelines #19953

emmazzz · 2024-10-22T07:57:03Z

Description

This PR adds schema and pipeline for various transaction indices including

tx_affected_addresses (replacing tx_senders and tx_recipients)
tx_kinds
tx_digests
tx_calls_fun (collapsing into one)

Test plan

cargo run -- \
--database-url "postgres://postgres:postgrespw@localhost:5432/sui_indexer_alt"
--remote-store-url https://checkpoints.mainnet.sui.io
--last-checkpoint 10000

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-10-22T07:57: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	Nov 2, 2024 11:29pm

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Nov 2, 2024 11:29pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Nov 2, 2024 11:29pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Nov 2, 2024 11:29pm

emmazzz · 2024-10-22T08:23:05Z

crates/sui-indexer-alt/migrations/2024-10-21-003426_tx_indices/up.sql

@@ -0,0 +1,39 @@
+CREATE TABLE IF NOT EXISTS tx_affected_addresses (
+    tx_sequence_number          BIGINT       NOT NULL,


What's our latest thought on NOT NULL? I think all the columns here are truly non nullable so I went with NOT NULL.

I think NOT NULL is fine for key columns (which are all the columns in the filter tables). In my PRs, I omitted the NOT NULL constraints on the columns that are part of the primary key, because being part of the primary key implies NOT NULL, but I'm happy to go back and change that -- I think not having the explicit NOT NULL was confusing in its own way.

Yes I think having explicit NOT NULL even for primary keys would be better.

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

amnn

LGTM thanks @emmazzz, just nits!

amnn · 2024-10-22T11:11:33Z

crates/sui-indexer-alt/migrations/2024-10-21-003426_tx_indices/up.sql

@@ -0,0 +1,39 @@
+CREATE TABLE IF NOT EXISTS tx_affected_addresses (
+    tx_sequence_number          BIGINT       NOT NULL,


I think NOT NULL is fine for key columns (which are all the columns in the filter tables). In my PRs, I omitted the NOT NULL constraints on the columns that are part of the primary key, because being part of the primary key implies NOT NULL, but I'm happy to go back and change that -- I think not having the explicit NOT NULL was confusing in its own way.

crates/sui-indexer-alt/migrations/2024-10-21-003426_tx_indices/up.sql

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

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

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

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

amnn · 2024-10-22T11:31:17Z

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

+#[derive(Debug, Clone)]
+pub enum TxKind {
+    SystemTransaction = 0,
+    ProgrammableTransaction = 1,
+}


nit: This should be under models::transactions.

Also, I wonder whether you could just store the TxKind directly, if you give it the correct repr? (I have no idea, genuinely curious whether diesel would pick that up).

Suggested change

#[derive(Debug, Clone)]

pub enum TxKind {

SystemTransaction = 0,

ProgrammableTransaction = 1,

}

#[derive(Debug, Clone)]

#[repr(i16)]

pub enum TxKind {

SystemTransaction = 0,

ProgrammableTransaction = 1,

}

repr does not seem to do much -- it only tells Rust how to store this enum in memory but casting is still necessary.

I know that we need to cast it if it's going into an i16 type, but I was wondering whether diesel would do something clever if in the model type, you used TxKind directly, and set its repr to i16.

crates/sui-indexer-alt/src/main.rs

crates/sui-indexer-alt/src/models/transactions.rs

gegaowp · 2024-10-22T14:44:55Z

crates/sui-indexer-alt/src/schema.rs

@@ -64,6 +72,30 @@ diesel::table! {
    }
 }

+diesel::table! {


curious is this now in sync with migrations and thus can be generated by diesel cmd?

Yes, this is generated using crates/sui-indexer-alt/generate_schema.sh and CI will complain if it mismatches.

amnn · 2024-10-23T13:16:03Z

crates/sui-indexer-alt/migrations/2024-10-21-003426_tx_indices/up.sql

+CREATE TABLE IF NOT EXISTS tx_digests (
+    tx_digest                   BYTEA        PRIMARY KEY,
+    tx_sequence_number          BIGINT       NOT NULL
+);
+CREATE INDEX IF NOT EXISTS tx_digests_tx_sequence_number_index ON tx_digests (tx_sequence_number);


I just realised something today -- because kv_transactions is now keyed by tx_digest this table should actually go in the opposite direction: It should be keyed by tx_sequence_number so that we can map sequence numbers to digests.

It's also a valid question whether we need this table at all. I.e. can we use tx_digest everywhere as the key for transactions. @emmazzz -- could you estimate what the size difference is between both approaches to decide which option is best?

I just realised something today -- because kv_transactions is now keyed by tx_digest this table should actually go in the opposite direction: It should be keyed by tx_sequence_number so that we can map sequence numbers to digests.

This part makes sense to me. When the user input is a list of transaction digests, we look them up directly in kv_transactions. And when user queries transactions via optional filters, we first look up the transaction sequence numbers using transaction indices, then use tx_digests to map the sequence numbers to the corresponding digests, and then finally look them up in kv_transactions via the digests. So that's three tables touched.

Writing this out, I see why you are thinking if we can use digest everywhere as this will allow us to remove one layer of indirection. But even if we use transaction digest everywhere, we will still need to store transaction sequence number everywhere as well for cursor and scan limit. That is one extra column (digest) for each row in five tables (affected objects + addresses, tx kinds, balance changes, tx calls fun), not to mention the indices we need to add here.

I suppose we can punt on this discussion right now and merge this PR in. I've created a linear task for it.

Ah yes good point on why we still want the sequence number. Indeed, happy to punt on whether we need to add tx_digest everywhere (and get rid of the explicit tx_digests table).

crates/sui-indexer-alt/migrations/2024-10-21-003426_tx_indices/up.sql

crates/sui-indexer-alt/src/models/transactions.rs

lxfind · 2024-10-23T15:19:50Z

crates/sui-indexer-alt/migrations/2024-10-21-003426_tx_indices/up.sql

+);
+CREATE INDEX IF NOT EXISTS tx_digests_tx_sequence_number_index ON tx_digests (tx_sequence_number);
+
+CREATE TABLE IF NOT EXISTS tx_kinds (


Curious what is the usage for this table?

GraphQL allows querying transactions filtered by transaction kind (system or programmable transaction). This table is used for that. And iirc performance investigation earlier this year determined that an index table would be needed to support such query.

(It's also a relatively small table: 170GB unpruned, or, 0.43% of the total footprint).

crates/sui-indexer-alt/migrations/2024-10-21-003426_tx_indices/up.sql

## Description This PR adds schema and pipeline for various transaction indices including - tx_affected_addresses (replacing tx_senders and tx_recipients) - tx_kinds - tx_digests - tx_calls_fun (collapsing into one) ## Test plan cargo run -- \ --database-url "postgres://postgres:postgrespw@localhost:5432/sui_indexer_alt" \ --remote-store-url https://checkpoints.mainnet.sui.io \ --last-checkpoint 10000 --- ## 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: --------- Co-authored-by: Ashok Menon <[email protected]>

emmazzz commented Oct 22, 2024

View reviewed changes

emmazzz requested review from amnn, gegaowp, lxfind, bmwill and wlmyng October 22, 2024 08:23

wlmyng approved these changes Oct 22, 2024

View reviewed changes

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

amnn approved these changes Oct 22, 2024

View reviewed changes

gegaowp reviewed Oct 22, 2024

View reviewed changes

amnn reviewed Oct 23, 2024

View reviewed changes

lxfind reviewed Oct 23, 2024

View reviewed changes

vercel bot deployed to Preview – sui-docs October 24, 2024 05:52 View deployment

vercel bot deployed to Preview – sui-docs October 24, 2024 07:21 View deployment

emmazzz enabled auto-merge (squash) October 24, 2024 07:26

amnn reviewed Oct 24, 2024

View reviewed changes

crates/sui-indexer-alt/migrations/2024-10-21-003426_tx_indices/up.sql Outdated Show resolved Hide resolved

emmazzz disabled auto-merge October 24, 2024 08:21

emmazzz and others added 2 commits November 2, 2024 23:27

[indexer-alt] add transaction indices

243ff05

fixup: address comments

6de71b3

amnn force-pushed the emma/tx-indices-alt branch from bb5d113 to 6de71b3 Compare November 2, 2024 23:28

amnn temporarily deployed to sui-typescript-aws-kms-test-env November 2, 2024 23:28 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 2, 2024 23:29 View deployment

amnn merged commit 61b97e6 into main Nov 3, 2024
52 checks passed

amnn deleted the emma/tx-indices-alt branch November 3, 2024 00:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[indexer-alt] add transaction indices pipelines #19953

[indexer-alt] add transaction indices pipelines #19953

emmazzz commented Oct 22, 2024

vercel bot commented Oct 22, 2024 •

edited

Loading

emmazzz Oct 22, 2024

amnn Oct 22, 2024

emmazzz Oct 24, 2024

amnn left a comment

amnn Oct 22, 2024

amnn Oct 22, 2024

amnn Oct 22, 2024

emmazzz Oct 24, 2024

amnn Oct 24, 2024

gegaowp Oct 22, 2024

amnn Oct 23, 2024

amnn Oct 23, 2024

emmazzz Oct 24, 2024

emmazzz Oct 24, 2024

amnn Oct 24, 2024

lxfind Oct 23, 2024

emmazzz Oct 24, 2024

amnn Oct 24, 2024

		@@ -0,0 +1,39 @@
		CREATE TABLE IF NOT EXISTS tx_affected_addresses (
		tx_sequence_number BIGINT NOT NULL,

[indexer-alt] add transaction indices pipelines #19953

[indexer-alt] add transaction indices pipelines #19953

Conversation

emmazzz commented Oct 22, 2024

Description

Test plan

Release notes

vercel bot commented Oct 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

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

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

vercel bot commented Oct 22, 2024 •

edited

Loading