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.

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

services/horizon/internal/db2/history: Fix claimable_balance_claimants subquery in GetClaimableBalances() #5207

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Feb 14, 2024

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

The request to /claimable_balances?claimant=GDFZV7N7WXJCHL27IX4CI6TQY7ZDHLXMLVX5XHOV6CPCQVUCGDKKFPGQ times out on staging (running horizon 2.28.2) but not on production (running horizon 2.28.0). The degradation in this endpoint is caused by bug introduced in #5200

The query corresponding to /claimable_balances?claimant=GDFZV7N7WXJCHL27IX4CI6TQY7ZDHLXMLVX5XHOV6CPCQVUCGDKKFPGQ in horizon 2.28.2 is:

SELECT
        cb.id, cb.claimants, cb.asset, cb.amount, cb.sponsor, cb.last_modified_ledger, cb.flags
FROM claimable_balances cb
WHERE cb.id IN (
        SELECT id FROM claimable_balance_claimants WHERE destination = 'GDFZV7N7WXJCHL27IX4CI6TQY7ZDHLXMLVX5XHOV6CPCQVUCGDKKFPGQ'
        ORDER BY cb.last_modified_ledger asc, cb.id asc
        LIMIT 10
)
ORDER BY cb.last_modified_ledger asc, cb.id asc LIMIT 10

The problem is in the subquery which selects from claimable_balance_claimants:

SELECT id FROM claimable_balance_claimants WHERE destination = 'GDFZV7N7WXJCHL27IX4CI6TQY7ZDHLXMLVX5XHOV6CPCQVUCGDKKFPGQ'
        ORDER BY cb.last_modified_ledger asc, cb.id asc
        LIMIT 10

Notice that the order by clause refers to the claimable_balances table. It should actually be referring to the claimable_balance_claimants table.

In this commit, the query is fixed to:

SELECT
       cb.id, cb.claimants, cb.asset, cb.amount, cb.sponsor, cb.last_modified_ledger, cb.flags
FROM claimable_balances cb
WHERE cb.id IN (
       SELECT claimable_balance_claimants.id FROM claimable_balance_claimants WHERE claimable_balance_claimants.destination = 'GDFZV7N7WXJCHL27IX4CI6TQY7ZDHLXMLVX5XHOV6CPCQVUCGDKKFPGQ'
       ORDER BY claimable_balance_claimants.last_modified_ledger asc, claimable_balance_claimants.id asc
       LIMIT 10
)
ORDER BY cb.last_modified_ledger asc, cb.id asc LIMIT 10

Now the subquery explicitly refers to claimable_balance_claimants instead of the claimable_balances table.

Known limitations

[N/A]

@tamirms tamirms requested a review from a team February 14, 2024 08:51
@tamirms
Copy link
Contributor Author

tamirms commented Feb 14, 2024

Query plan for the old query:

horizon=> EXPLAIN SELECT
        cb.id, cb.claimants, cb.asset, cb.amount, cb.sponsor, cb.last_modified_ledger, cb.flags
FROM claimable_balances cb
WHERE cb.id IN (
        SELECT id FROM claimable_balance_claimants WHERE destination = 'GDFZV7N7WXJCHL27IX4CI6TQY7ZDHLXMLVX5XHOV6CPCQVUCGDKKFPGQ'
        ORDER BY cb.last_modified_ledger asc, cb.id asc
        LIMIT 10
)
ORDER BY cb.last_modified_ledger asc, cb.id asc LIMIT 10;
                                                                                    QUERY PLAN                                                                                    
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Limit  (cost=0.68..61.79 rows=10 width=517)
   ->  Index Scan using claimable_balances_by_last_modified_ledger_and_id on claimable_balances cb  (cost=0.68..20152492.23 rows=3297608 width=517)
         Filter: (SubPlan 1)
         SubPlan 1
           ->  Limit  (cost=0.69..5.13 rows=10 width=109)
                 ->  Index Only Scan using claimable_balance_claimants_by_destination_last_modified_ledger on claimable_balance_claimants  (cost=0.69..107.77 rows=241 width=109)
                       Index Cond: (destination = 'GDFZV7N7WXJCHL27IX4CI6TQY7ZDHLXMLVX5XHOV6CPCQVUCGDKKFPGQ'::text)
(7 rows)

Query plan for the new query (containing the bug fix):

horizon=> EXPLAIN SELECT
       cb.id, cb.claimants, cb.asset, cb.amount, cb.sponsor, cb.last_modified_ledger, cb.flags
FROM claimable_balances cb
WHERE cb.id IN (
       SELECT claimable_balance_claimants.id FROM claimable_balance_claimants WHERE claimable_balance_claimants.destination = 'GDFZV7N7WXJCHL27IX4CI6TQY7ZDHLXMLVX5XHOV6CPCQVUCGDKKFPGQ'
       ORDER BY claimable_balance_claimants.last_modified_ledger asc, claimable_balance_claimants.id asc
       LIMIT 10
)
ORDER BY cb.last_modified_ledger asc, cb.id asc LIMIT 10
;
                                                                                        QUERY PLAN                                                                                         
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Limit  (cost=31.27..31.29 rows=10 width=517)
   ->  Sort  (cost=31.27..31.29 rows=10 width=517)
         Sort Key: cb.last_modified_ledger, cb.id
         ->  Nested Loop  (cost=5.81..31.10 rows=10 width=517)
               ->  HashAggregate  (cost=5.25..5.35 rows=10 width=73)
                     Group Key: claimable_balance_claimants.id
                     ->  Limit  (cost=0.69..5.13 rows=10 width=77)
                           ->  Index Only Scan using claimable_balance_claimants_by_destination_last_modified_ledger on claimable_balance_claimants  (cost=0.69..107.77 rows=241 width=77)
                                 Index Cond: (destination = 'GDFZV7N7WXJCHL27IX4CI6TQY7ZDHLXMLVX5XHOV6CPCQVUCGDKKFPGQ'::text)
               ->  Index Scan using claimable_balances_pkey on claimable_balances cb  (cost=0.56..2.58 rows=1 width=517)
                     Index Cond: (id = claimable_balance_claimants.id)
(11 rows)


Where(
sq.Expr(
fmt.Sprintf("(%s.last_modified_ledger, %s.id) > (?, ?)", tableName, tableName),
Copy link
Contributor

@sreuland sreuland Feb 14, 2024

Choose a reason for hiding this comment

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

embedded sql text fragment assemblies become so tedious, difficult to recognize the overall sql statement, not suggesting anything here, just mentioning go-jet again, to see if type-safe sql bindings re-resonate with anyone. the concept was actually pitched in a future ideas worksheet a ways back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that's a good idea. I think it's worth reviving go-jet

@tamirms tamirms merged commit 4cc1350 into stellar:release-horizon-v2.28.3 Feb 14, 2024
26 checks passed
@tamirms tamirms deleted the fix-cb-subquery branch February 14, 2024 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants