perf(postgres:subaccounts): add partial indices and optimize query fo…#3318
Merged
Conversation
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a migration creating partial NOT NULL indices on transfers; converts multiple SQL queries to use parameterized bindings and EXISTS/UNION ALL patterns; wraps certain DB queries in transactions with SET LOCAL work_mem; widens binding types to support object-based parameter maps; serializes test inserts for determinism; changes one helper to read from primary DB; and applies minor SQL formatting fixes. ChangesDatabase indices and parameterized queries
Transaction management and service changes
Test and formatting updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@indexer/packages/postgres/src/stores/subaccount-table.ts`:
- Around line 110-124: The SQL currently interpolates createdBeforeOrAtHeight
directly into queryString; change the query to use parameter placeholders (e.g.,
replace '${createdBeforeOrAtHeight}' with ? in both EXISTS clauses) and pass
createdBeforeOrAtHeight via the rawQuery bindings (ensure options.bindings is an
array and include the value twice to match the two placeholders). Update the
code that calls rawQuery so it supplies options.bindings =
[createdBeforeOrAtHeight, createdBeforeOrAtHeight] (or append if bindings exist)
so knex.raw() receives the parameterized values.
🧹 Nitpick comments (1)
indexer/packages/postgres/src/db/migrations/migration_files/20260116114440_create_indices_transfers_not_null.ts (1)
4-16: ConsiderCONCURRENTLYto avoid blocking writes on large tables.Given
transaction: false, this migration is eligible forDROP INDEX CONCURRENTLY/CREATE INDEX CONCURRENTLYto reduce lock impact. Note:CONCURRENTLYrequires each statement to be executed separately (not in a single multi-statementknex.raw).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
indexer/packages/postgres/src/db/migrations/migration_files/20260116114440_create_indices_transfers_not_null.tsindexer/packages/postgres/src/stores/subaccount-table.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 2815
File: indexer/packages/postgres/src/db/migrations/migration_files/20250423144330_add_twap_fields_to_orders_and_fills_table.ts:40-71
Timestamp: 2025-08-13T14:27:43.370Z
Learning: The `formatAlterTableEnumSql` helper function in the dydxprotocol/v4-chain indexer uses PostgreSQL's `NOT VALID` clause when creating CHECK constraints, which means the constraints only apply to new data and do not validate existing rows. This allows enum-like constraint updates to proceed safely without failing on existing data that might not conform to the new constraint values.
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 2815
File: indexer/packages/postgres/src/db/migrations/migration_files/20250428122430_add_twap_types_to_order_table.ts:0-0
Timestamp: 2025-08-11T15:03:01.902Z
Learning: The dydxprotocol/v4-chain indexer uses CHECK constraints (not PostgreSQL native enum types) for enum-like columns in the orders table. The `formatAlterTableEnumSql` helper function in the indexer/packages/postgres migrations drops and recreates CHECK constraints with the pattern `${tableName}_${columnName}_check` to enforce allowed values on text columns.
📚 Learning: 2024-11-15T16:00:11.304Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:852-865
Timestamp: 2024-11-15T16:00:11.304Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
indexer/packages/postgres/src/stores/subaccount-table.ts
📚 Learning: 2024-11-15T15:59:28.095Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:833-850
Timestamp: 2024-11-15T15:59:28.095Z
Learning: The function `GetInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
indexer/packages/postgres/src/stores/subaccount-table.ts
📚 Learning: 2025-08-11T15:03:01.902Z
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 2815
File: indexer/packages/postgres/src/db/migrations/migration_files/20250428122430_add_twap_types_to_order_table.ts:0-0
Timestamp: 2025-08-11T15:03:01.902Z
Learning: The dydxprotocol/v4-chain indexer uses CHECK constraints (not PostgreSQL native enum types) for enum-like columns in the orders table. The `formatAlterTableEnumSql` helper function in the indexer/packages/postgres migrations drops and recreates CHECK constraints with the pattern `${tableName}_${columnName}_check` to enforce allowed values on text columns.
Applied to files:
indexer/packages/postgres/src/db/migrations/migration_files/20260116114440_create_indices_transfers_not_null.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: test / run_command
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: check-build-auxo
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: build-and-push-mainnet
- GitHub Check: run_command
- GitHub Check: lint
- GitHub Check: build-and-push-testnet
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
testnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
UnbornAztecKing
temporarily deployed
to
mainnet
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@indexer/packages/postgres/src/stores/subaccount-table.ts`:
- Line 15: The import list includes an unused symbol IsolationLevel in
subaccount-table.ts; remove IsolationLevel from the import statement so it is no
longer imported (update the import that currently lists IsolationLevel), save
and run the linter/TS compile to ensure no other references remain.
- Around line 112-149: The function currently always calls Transaction.start()
and commits/rolls back that txId which breaks callers that pass an active
transaction; change it to reuse an existing transaction when options.txId is
provided: set const externalTxId = options?.txId; const startedHere =
!externalTxId; const txId = externalTxId ?? await Transaction.start(); build
txOptions = { ...options, txId } and use that for rawQuery, and only call
Transaction.commit(txId) or Transaction.rollback(txId) when startedHere is true
(if reusing options.txId, do not commit/rollback). Keep the existing use of
rawQuery and bindings unchanged.
🧹 Nitpick comments (3)
indexer/packages/postgres/src/stores/transfer-table.ts (2)
478-498: Inconsistent parameterization:getNetTransfersBetweenBlockHeightsForSubaccountstill uses string interpolation.This function still interpolates
subaccountId,createdAfterHeight, andcreatedBeforeOrAtHeightdirectly into the SQL string (lines 483, 491-495), whilegetNetTransfersPerSubaccountwas just refactored to use safe bindings. Consider applying the same parameterization pattern here for consistency and to prevent SQL injection.♻️ Suggested parameterization
export async function getNetTransfersBetweenBlockHeightsForSubaccount( subaccountId: string, createdAfterHeight: string, createdBeforeOrAtHeight: string, options: Options = DEFAULT_POSTGRES_OPTIONS, ): Promise<AssetTransferMap> { const queryString: string = ` SELECT "assetId", SUM( CASE - WHEN "senderSubaccountId" = '${subaccountId}' THEN -"size" + WHEN "senderSubaccountId" = :subaccountId THEN -"size" ELSE "size" END ) AS "totalSize" FROM "transfers" WHERE ( - "senderSubaccountId" = '${subaccountId}' - OR "recipientSubaccountId" = '${subaccountId}' + "senderSubaccountId" = :subaccountId + OR "recipientSubaccountId" = :subaccountId ) - AND "createdAtHeight" > ${createdAfterHeight} - AND "createdAtHeight" <= ${createdBeforeOrAtHeight} + AND "createdAtHeight" > :createdAfterHeight::bigint + AND "createdAtHeight" <= :createdBeforeOrAtHeight::bigint GROUP BY "assetId"; `; - const result: { - rows: { - assetId: string, - totalSize: string, - }[], - } = await rawQuery(queryString, options); + const result: { + rows: { + assetId: string, + totalSize: string, + }[], + } = await rawQuery(queryString, { + ...options, + bindings: { subaccountId, createdAfterHeight, createdBeforeOrAtHeight }, + });
558-584: Inconsistent parameterization:getNetTransfersBetweenSubaccountIdsalso uses string interpolation.This function interpolates
assetId,sourceSubaccountId, andrecipientSubaccountIddirectly into SQL. For consistency with the improvedgetNetTransfersPerSubaccountand to mitigate SQL injection risk, consider using bindings here as well.♻️ Suggested parameterization
export async function getNetTransfersBetweenSubaccountIds( sourceSubaccountId: string, recipientSubaccountId: string, assetId: string, options: Options = DEFAULT_POSTGRES_OPTIONS, ): Promise<string> { const queryString: string = ` SELECT COALESCE(SUM(s."size"), '0') AS "totalSize" FROM ( SELECT DISTINCT "size" AS "size", "id" FROM "transfers" - WHERE "transfers"."assetId" = '${assetId}' - AND "transfers"."senderSubaccountId" = '${sourceSubaccountId}' - AND "transfers"."recipientSubaccountId" = '${recipientSubaccountId}' + WHERE "transfers"."assetId" = :assetId + AND "transfers"."senderSubaccountId" = :sourceSubaccountId + AND "transfers"."recipientSubaccountId" = :recipientSubaccountId UNION SELECT DISTINCT -"size" AS "size", "id" FROM "transfers" - WHERE "transfers"."assetId" = '${assetId}' - AND "transfers"."senderSubaccountId" = '${recipientSubaccountId}' - AND "transfers"."recipientSubaccountId" = '${sourceSubaccountId}' + WHERE "transfers"."assetId" = :assetId + AND "transfers"."senderSubaccountId" = :recipientSubaccountId + AND "transfers"."recipientSubaccountId" = :sourceSubaccountId ) AS s `; - const result: { - rows: { totalSize: string }[], - } = await rawQuery(queryString, options); + const result: { + rows: { totalSize: string }[], + } = await rawQuery(queryString, { + ...options, + bindings: { assetId, sourceSubaccountId, recipientSubaccountId }, + });indexer/packages/postgres/src/db/migrations/migration_files/20260116114440_create_indices_transfers_not_null.ts (1)
3-22: Consider usingDROP INDEX CONCURRENTLYinup()for consistency.The
upmigration uses non-concurrentDROP INDEX(lines 5, 15) whiledownusesDROP INDEX CONCURRENTLY(lines 27, 36). While the synchronous drop inupmay be intentional (old indexes might drop quickly), usingCONCURRENTLYuniformly would minimize lock contention during migration, especially on a largetransferstable.Note: There's an inherent risk window between DROP and CREATE where queries may run slower. The idempotent
IF EXISTS/IF NOT EXISTSclauses provide good retry safety if the migration needs to be rerun.♻️ Optional: Use concurrent DROP in up()
export async function up(knex: Knex): Promise<void> { await knex.raw( - 'DROP INDEX IF EXISTS "transfers_sendersubaccountid_createdatheight_index";', + 'DROP INDEX CONCURRENTLY IF EXISTS "transfers_sendersubaccountid_createdatheight_index";', ); await knex.raw( `CREATE INDEX CONCURRENTLY IF NOT EXISTS transfers_sender_id_height_nn ON transfers("senderSubaccountId", "createdAtHeight") WHERE "senderSubaccountId" IS NOT NULL;`, ); await knex.raw( - 'DROP INDEX IF EXISTS "transfers_recipientsubaccountid_createdatheight_index";', + 'DROP INDEX CONCURRENTLY IF EXISTS "transfers_recipientsubaccountid_createdatheight_index";', ); await knex.raw( `CREATE INDEX CONCURRENTLY IF NOT EXISTS transfers_recipient_id_height_nn ON transfers("recipientSubaccountId", "createdAtHeight") WHERE "recipientSubaccountId" IS NOT NULL;`, ); }
| SubaccountQueryConfig, | ||
| SubaccountColumns, | ||
| SubaccountCreateObject, | ||
| IsolationLevel, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "IsolationLevel" indexer/packages/postgres/src/stores/subaccount-table.tsRepository: dydxprotocol/v4-chain
Length of output: 85
Remove unused import IsolationLevel.
IsolationLevel is imported but never referenced in this file.
🤖 Prompt for AI Agents
In `@indexer/packages/postgres/src/stores/subaccount-table.ts` at line 15, The
import list includes an unused symbol IsolationLevel in subaccount-table.ts;
remove IsolationLevel from the import statement so it is no longer imported
(update the import that currently lists IsolationLevel), save and run the
linter/TS compile to ensure no other references remain.
Comment on lines
+112
to
149
| const txId: number = await Transaction.start(); | ||
| const txOptions: Options = { | ||
| ...options, | ||
| txId, | ||
| }; | ||
|
|
||
| try { | ||
| await rawQuery('SET LOCAL work_mem = \'64MB\';', txOptions); | ||
|
|
||
| const result: { rows: SubaccountFromDatabase[] } = await rawQuery(` | ||
| SELECT s.* | ||
| FROM subaccounts s | ||
| WHERE EXISTS ( | ||
| SELECT 1 | ||
| FROM transfers t | ||
| WHERE t."senderSubaccountId" = s.id | ||
| AND t."createdAtHeight" <= :createdBeforeOrAtHeight::bigint | ||
| ) | ||
| OR EXISTS ( | ||
| SELECT 1 | ||
| FROM transfers t | ||
| WHERE t."recipientSubaccountId" = s.id | ||
| AND t."createdAtHeight" <= :createdBeforeOrAtHeight::bigint | ||
| )`, | ||
| { | ||
| ...txOptions, | ||
| bindings: { createdBeforeOrAtHeight }, | ||
| }); | ||
|
|
||
| await Transaction.commit(txId); | ||
|
|
||
| return result.rows; | ||
| return result.rows; | ||
|
|
||
| } catch (error) { | ||
| await Transaction.rollback(txId); | ||
| throw error; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -B 2 -A 5 "getSubaccountsWithTransfers" --type tsRepository: dydxprotocol/v4-chain
Length of output: 7682
🏁 Script executed:
rg -n -B 10 -A 10 'getSubaccountsWithTransfers.*txId' indexer/services/roundtable/src/helpers/pnl-ticks-helper.tsRepository: dydxprotocol/v4-chain
Length of output: 843
🏁 Script executed:
rg -n -B 30 'getMostRecentPnlTicksForEachAccount\(\)' indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts | head -60Repository: dydxprotocol/v4-chain
Length of output: 2107
🏁 Script executed:
rg -n -B 5 -A 10 'function getPnlTicksCreateObjects' indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts | head -40Repository: dydxprotocol/v4-chain
Length of output: 700
🏁 Script executed:
# Check how BlockTable.getLatest handles txId
rg -n -A 15 'export async function getLatest' indexer/packages/postgres/src/stores/block-table.tsRepository: dydxprotocol/v4-chain
Length of output: 609
🏁 Script executed:
# Check the Transaction class to understand txId behavior
rg -n -A 20 'class Transaction|export.*function start' indexer/packages/postgres/src/utils/transaction.ts | head -50Repository: dydxprotocol/v4-chain
Length of output: 155
🏁 Script executed:
# Check how setupBaseQuery uses txId to verify if it's reused or created
rg -n -A 15 'setupBaseQuery' indexer/packages/postgres/src/utils/helpers.ts | head -40Repository: dydxprotocol/v4-chain
Length of output: 151
🏁 Script executed:
# Find Transaction-related files
fd -t f 'transaction' indexer/packages/postgres/src/Repository: dydxprotocol/v4-chain
Length of output: 393
🏁 Script executed:
# Find helper files
fd -t f 'helper' indexer/packages/postgres/src/ | head -20Repository: dydxprotocol/v4-chain
Length of output: 484
🏁 Script executed:
# Check the Transaction class implementation
rg -n -A 20 'export.*class Transaction|export.*function start' indexer/packages/postgres/src/helpers/transaction.ts | head -80Repository: dydxprotocol/v4-chain
Length of output: 913
🏁 Script executed:
# Check how other similar functions in subaccount-table.ts handle the txId parameter
rg -n -B 3 -A 10 'export async function' indexer/packages/postgres/src/stores/subaccount-table.ts | head -100Repository: dydxprotocol/v4-chain
Length of output: 2900
🏁 Script executed:
# Check how getPnlTicksCreateObjects is called
rg -n -B 5 -A 5 'getPnlTicksCreateObjects' indexer/services/roundtable/src/tasks/pnl-instrumentation.tsRepository: dydxprotocol/v4-chain
Length of output: 47
🏁 Script executed:
# Search for calls to getPnlTicksCreateObjects across the codebase
rg -n 'getPnlTicksCreateObjects' indexer/Repository: dydxprotocol/v4-chain
Length of output: 1556
🏁 Script executed:
# Check the create-pnl-ticks.ts to understand the transaction context
rg -n -B 10 -A 10 'getPnlTicksCreateObjects\(txId\)' indexer/services/roundtable/src/tasks/create-pnl-ticks.tsRepository: dydxprotocol/v4-chain
Length of output: 830
Fix transaction handling: reuse existing txId instead of creating a new transaction.
The function creates a new transaction and ignores any txId provided in options. When called from getPnlTicksCreateObjects(txId) (which is initiated with REPEATABLE_READ isolation), this breaks transaction consistency by starting a separate transaction instead of reusing the caller's context. Other functions in the same file (create, update, findById) correctly use Transaction.get(options.txId) to reuse the transaction.
Update the function to check if options.txId is provided and reuse it; only start a new transaction if none is provided.
🤖 Prompt for AI Agents
In `@indexer/packages/postgres/src/stores/subaccount-table.ts` around lines 112 -
149, The function currently always calls Transaction.start() and commits/rolls
back that txId which breaks callers that pass an active transaction; change it
to reuse an existing transaction when options.txId is provided: set const
externalTxId = options?.txId; const startedHere = !externalTxId; const txId =
externalTxId ?? await Transaction.start(); build txOptions = { ...options, txId
} and use that for rawQuery, and only call Transaction.commit(txId) or
Transaction.rollback(txId) when startedHere is true (if reusing options.txId, do
not commit/rollback). Keep the existing use of rawQuery and bindings unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@indexer/services/ender/src/lib/on-message.ts`:
- Line 89: The SET LOCAL work_mem query is executed before the transaction try
block, so if it throws the transaction will leak; move the rawQuery('SET LOCAL
work_mem=\'128MB\';', { txId }) call (and the isolation level setting call that
configures the transaction) into the existing try block inside the function
handling the transaction in on-message.ts so that any failure triggers the
existing catch/finally rollback logic (locate the rawQuery invocation and the
surrounding try/catch that runs rollback/finally and relocate the SET LOCAL and
isolation-level rawQuery calls to the top of that try block).
🧹 Nitpick comments (1)
indexer/services/ender/src/lib/on-message.ts (1)
14-14: Prefer public postgres exports over deep build-path imports (Line 14).
Importing frombuild/src/...couples this service to internal package layout; consider re-exportingrawQueryfrom@dydxprotocol-indexer/postgres(or a stable public module) and importing from there.
| let success: boolean = false; | ||
| const txId: number = await Transaction.start(); | ||
| await Transaction.setIsolationLevel(txId, IsolationLevel.READ_UNCOMMITTED); | ||
| await rawQuery('SET LOCAL work_mem=\'128MB\';', { txId }); |
Contributor
There was a problem hiding this comment.
Ensure SET LOCAL failure triggers rollback (Line 89).
rawQuery runs before the try, so an error here skips rollback/finally and can leak the transaction. Move it (and isolation level) into the try block.
✅ Suggested fix
let success: boolean = false;
const txId: number = await Transaction.start();
- await Transaction.setIsolationLevel(txId, IsolationLevel.READ_UNCOMMITTED);
- await rawQuery('SET LOCAL work_mem=\'128MB\';', { txId });
try {
+ await Transaction.setIsolationLevel(txId, IsolationLevel.READ_UNCOMMITTED);
+ await rawQuery('SET LOCAL work_mem=\'128MB\';', { txId });
validateIndexerTendermintBlock(indexerTendermintBlock);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await rawQuery('SET LOCAL work_mem=\'128MB\';', { txId }); | |
| let success: boolean = false; | |
| const txId: number = await Transaction.start(); | |
| try { | |
| await Transaction.setIsolationLevel(txId, IsolationLevel.READ_UNCOMMITTED); | |
| await rawQuery('SET LOCAL work_mem=\'128MB\';', { txId }); | |
| validateIndexerTendermintBlock(indexerTendermintBlock); |
🤖 Prompt for AI Agents
In `@indexer/services/ender/src/lib/on-message.ts` at line 89, The SET LOCAL
work_mem query is executed before the transaction try block, so if it throws the
transaction will leak; move the rawQuery('SET LOCAL work_mem=\'128MB\';', { txId
}) call (and the isolation level setting call that configures the transaction)
into the existing try block inside the function handling the transaction in
on-message.ts so that any failure triggers the existing catch/finally rollback
logic (locate the rawQuery invocation and the surrounding try/catch that runs
rollback/finally and relocate the SET LOCAL and isolation-level rawQuery calls
to the top of that try block).
Kefancao
approved these changes
Jun 23, 2026
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
…r subaccounts w/transfers
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@indexer/packages/postgres/src/stores/subaccount-table.ts`:
- Around line 111-115: In the subaccount-table.ts file, the code unconditionally
calls Transaction.start() to create a txId bound to the primary connection, then
merges it into txOptions which may already contain readReplica: true from the
caller. This creates a routing conflict. Before calling Transaction.start(),
check if the incoming options parameter contains readReplica: true. If it does,
either throw an error indicating that transactions cannot be used with read
replicas, or skip starting the transaction entirely. Ensure that txOptions never
contains both a txId (primary transaction) and readReplica: true simultaneously.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 525d45d3-76ce-49f0-8e75-94af0e06f030
📒 Files selected for processing (9)
indexer/packages/postgres/__tests__/stores/funding-payments-table.test.tsindexer/packages/postgres/src/db/migrations/migration_files/20260116114440_create_indices_transfers_not_null.tsindexer/packages/postgres/src/stores/candle-table.tsindexer/packages/postgres/src/stores/fill-table.tsindexer/packages/postgres/src/stores/subaccount-table.tsindexer/packages/postgres/src/stores/transfer-table.tsindexer/packages/postgres/src/types/utility-types.tsindexer/services/ender/src/lib/on-message.tsindexer/services/roundtable/src/helpers/pnl-ticks-helper.ts
✅ Files skipped from review due to trivial changes (3)
- indexer/packages/postgres/src/stores/candle-table.ts
- indexer/packages/postgres/src/types/utility-types.ts
- indexer/packages/postgres/src/stores/fill-table.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts
- indexer/packages/postgres/tests/stores/funding-payments-table.test.ts
- indexer/services/ender/src/lib/on-message.ts
- indexer/packages/postgres/src/db/migrations/migration_files/20260116114440_create_indices_transfers_not_null.ts
- indexer/packages/postgres/src/stores/transfer-table.ts
Comment on lines
+111
to
+115
| const txId: number = await Transaction.start(); | ||
| const txOptions: Options = { | ||
| ...options, | ||
| txId, | ||
| }; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect transaction startup and readReplica+txId usage to verify
# getSubaccountsWithTransfers preserves read-replica routing.
set -euo pipefail
echo "### Transaction helper implementation"
fd -t f 'transaction\.ts$' indexer/packages/postgres/src --exec sh -c '
echo "## $1"
sed -n "1,220p" "$1"
' sh {}
echo
echo "### Call sites that combine Transaction.start, txId, and readReplica"
rg -n -C4 'Transaction\.start\(|readReplica:\s*true|txId' indexer/packages/postgres/src indexer/servicesRepository: dydxprotocol/v4-chain
Length of output: 50378
🏁 Script executed:
# Find how readReplica option is used in queries
rg -n "readReplica" indexer/packages/postgres/src --type ts -A 2 | head -100
# Check if there's special handling for transactions + read-replicas
rg -n "Model\.startTransaction|setupBaseQuery" indexer/packages/postgres/src/stores -A 5 | head -80
# Look for rawQuery implementation to see how readReplica is handled
fd -t f 'raw-query|rawQuery' indexer/packages/postgres/src --exec head -50 {} \;Repository: dydxprotocol/v4-chain
Length of output: 9236
🏁 Script executed:
# Check the full setupBaseQuery implementation
rg -n "setupBaseQuery" indexer/packages/postgres/src/helpers/stores-helpers.ts -A 15
# Check the full rawQuery implementation
rg -n "rawQuery" indexer/packages/postgres/src/helpers/stores-helpers.ts -B 5 -A 15
# See if there's any conflict detection or special handling when txId + readReplica are both present
rg -n "txId.*readReplica|readReplica.*txId" indexer/packages/postgres/src -B 3 -A 3Repository: dydxprotocol/v4-chain
Length of output: 1392
🏁 Script executed:
# Check the full rawQuery implementation to see if there's error handling
cat indexer/packages/postgres/src/helpers/stores-helpers.ts | head -100
# Check what Transaction.start() does and if it can be configured
cat indexer/packages/postgres/src/helpers/transaction.ts
# Find if getSubaccountsWithTransfers is actually called with readReplica option
rg -n "getSubaccountsWithTransfers" indexer --type ts -B 2 -A 2Repository: dydxprotocol/v4-chain
Length of output: 9162
🏁 Script executed:
# Check what happens in the pnl-ticks-helper when it passes both readReplica and txId
sed -n '60,85p' indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts
# Look at the full getSubaccountsWithTransfers function to understand the transaction lifecycle
sed -n '106,148p' indexer/packages/postgres/src/stores/subaccount-table.tsRepository: dydxprotocol/v4-chain
Length of output: 2120
🏁 Script executed:
# Check if Model.startTransaction() has any connection configuration
rg -n "Model.startTransaction|knexPrimary.*startTransaction" indexer/packages/postgres/src -B 3 -A 3
# Check what knexReadReplica.getConnection() returns and if it supports transactions
fd -t f 'knex\.ts$' indexer/packages/postgres/src --exec cat {} \;Repository: dydxprotocol/v4-chain
Length of output: 1926
Transaction started on primary connection conflicts with caller's read-replica routing preference.
Line 111 unconditionally starts a transaction on the primary connection using Transaction.start(), which calls Model.startTransaction() (bound globally to knexPrimary). When callers pass { readReplica: true } in options (as seen at indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts:73), the resulting txOptions contains both txId (primary transaction) and readReplica: true.
In rawQuery(), this creates a routing mismatch: the connection is selected as the read replica, but the transaction object is bound to the primary. This prevents proper transaction isolation on the replica or forces the query to the primary despite the caller's read-replica intent.
Either:
- Reject
readReplica: truewhen a transaction ID is provided, or - Do not start a transaction if
readReplica: trueis passed and notxIdis already present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indexer/packages/postgres/src/stores/subaccount-table.ts` around lines 111 -
115, In the subaccount-table.ts file, the code unconditionally calls
Transaction.start() to create a txId bound to the primary connection, then
merges it into txOptions which may already contain readReplica: true from the
caller. This creates a routing conflict. Before calling Transaction.start(),
check if the incoming options parameter contains readReplica: true. If it does,
either throw an error indicating that transactions cannot be used with read
replicas, or skip starting the transaction entirely. Ensure that txOptions never
contains both a txId (primary transaction) and readReplica: true simultaneously.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
transferswith partial indexes that excludeNULLsubaccount IDs to reduce index size and improve write performance.getSubaccountsWithTransfersquery fromIN (UNION)pattern toEXISTSpattern for better query planning and performance.Details
Database schema
transfers_sender_id_height_nnandtransfers_recipient_id_height_nnon(senderSubaccountId, createdAtHeight)and(recipientSubaccountId, createdAtHeight)withWHERE ... IS NOT NULLpredicates.transfers_sendersubaccountid_createdatheight_indexandtransfers_recipientsubaccountid_createdatheight_index(full-table versions).transaction: falseto avoid long-held locks during index rebuild.Query optimization
WHERE id IN (SELECT senderSubaccountId ... UNION SELECT recipientSubaccountId ...)WHERE EXISTS (SELECT 1 ... senderSubaccountId = s.id) OR EXISTS (SELECT 1 ... recipientSubaccountId = s.id)Risk & Impact
DROP IF EXISTS/CREATE IF NOT EXISTSidempotency for safe retries.Testing
getSubaccountsWithTransferscover functional correctness.Summary by CodeRabbit
Bug Fixes
NULLsender/recipient subaccount fields and aligning indexing with non-null values.Refactor
transfersindexes with partial indexes for faster lookups.Tests
Style