sqldb: move native SQL payments migrations into mainline#10627
sqldb: move native SQL payments migrations into mainline#10627ziggie1984 wants to merge 12 commits intolightningnetwork:masterfrom
Conversation
The invoice tombstone acts as a system wide kv db tombstone so there is no need for a specific payment tombstone. Moreover a TODO is added to redesign the current setting of the invoice tombstone because it is also fragile to crashes after the tombstone is set and the sql transaction of the migration fails to commit. Additionally the missing cleanup calls are added in case we return early because of an error.
During route retrieval don't query for the channel capacity. We default to the static incomingAmt of the route. That was already done previously when the channel was closed or private. The channel capacity has been deprecated for quite a while so it is acceptable to avoid the performance hit querying the graph db. In the next release this field will be removed.
The previous commit stopped setting the channel capacity when retrieving the route. This commit makes sure that in the next release we remove the entries from the rpc interface.
Since migration 10 is already merged into master it cannot be edited. Add a new migration (000013_payments_index_improvements) that carries forward two index improvements: - Drop idx_htlc_attempt_index on payment_htlc_attempts(attempt_index) and idx_route_hops_htlc_attempt_index on payment_route_hops(htlc_attempt_index). Both are redundant with existing UNIQUE constraints and only add write/maintenance overhead. - Add idx_htlc_payment_id_attempt_time on payment_htlc_attempts(payment_id, attempt_time) to optimise batched attempt reads that filter by payment_id and order by attempt_time (FetchHtlcAttemptsForPayments). - Add idx_htlc_resolutions_type_attempt_index on payment_htlc_attempt_resolutions(resolution_type, attempt_index) to optimise the failed-attempt cleanup path that filters by resolution_type before joining on attempt_index (DeleteFailedAttempts).
Add a new omit_hops field to ListPaymentsRequest that allows clients to skip loading hop-level route data for HTLC attempts, reducing both query cost and response size. When set, the route is returned with only route-level fields (TotalTimeLock, TotalAmount, SourcePubKey) and no individual hop data or hop-level custom records.
The previous query used an IN subquery that scanned all failed resolutions across all payments (O(N) where N = total failed attempts globally). Replace with a correlated EXISTS subquery that only checks resolutions for the specific payment's attempts, making it O(k) where k = attempts for this payment (typically 1-5).
fixes it for the orignal file and the code migration package where the code was just copied over.
Before this change the migration progress log only showed absolute counts and a cumulative average rate. This adds two improvements: - A cheap pre-count pass over the payments index bucket before migration starts, giving an upper-bound estimate of the total entries to migrate (noted as approximate since duplicates are also indexed). This allows showing a percentage complete on each progress line. - A rolling 30s window rate for the ETA calculation instead of the cumulative average rate. The cumulative rate is slow to react when throughput changes mid-migration; the rolling window makes the ETA responsive to recent conditions. On window reset the previous window's rate is used as a fallback for one tick to avoid a gap in the ETA. Example progress line after this change: Progress: 500000 payments (~48.5%), 499860 attempts | Rate: 399.2 pmt/s | Elapsed: 20m50s | ETA: ~22m8s
Replace direct `err != sql.ErrNoRows` comparison with `errors.Is` and extract the repeated fetch-and-check logic into a helper to reduce duplication across the sequential and concurrent benchmarks.
The native SQL payments migrations were previously gated behind test build tags in migrations_dev.go. This commit promotes them into the main migration sequence in migrations.go, making them available in production builds. The following migrations are moved to mainline: - 000010_payments (v12): initial payments SQL schema - 000011_payment_duplicates (v13): duplicate payment support - kv_payments_migration (v14): optional KV to SQL payment migration - 000012_drop_redundant_invoice_indexes (v15): index cleanup - 000013_payments_index_improvements (v16): payment index optimizations
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates the native SQL payments schema migrations into the core application, enabling their use in production builds. It also introduces a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request moves the native SQL payments schema migrations into the mainline, making them available in production builds. It also includes several related improvements, such as adding an omit_hops flag to the listpayments command for better performance, optimizing SQL queries, and improving migration progress reporting. The changes are well-structured and consistent across the codebase. I have one suggestion to improve a comment for clarity.
Note: Security Review did not run due to the size of the PR.
| // Avoid per-hop graph lookups by using the incoming amount as a | ||
| // lower bound for the capacity. This is not the actual channel | ||
| // capacity, but it is a reasonable approximation that avoids | ||
| // slow graph lookups and works for closed/private channels too. |
There was a problem hiding this comment.
The comment states that the incoming amount is used as a lower bound for the capacity. However, the incomingAmt variable is initialized to route.TotalAmount before the loop and is no longer updated within it. This means the total route amount is used for every hop, not the specific incoming amount for that hop. To avoid confusion, I suggest clarifying in the comment that the total route amount is being used.
| // Avoid per-hop graph lookups by using the incoming amount as a | |
| // lower bound for the capacity. This is not the actual channel | |
| // capacity, but it is a reasonable approximation that avoids | |
| // slow graph lookups and works for closed/private channels too. | |
| // Avoid per-hop graph lookups by using the total route amount as a | |
| // lower bound for the capacity. This is not the actual channel | |
| // capacity, but it is a reasonable approximation that avoids | |
| // slow graph lookups and works for closed/private channels too. |
There was a problem hiding this comment.
this was acutally a bug solved in #10535, we want the incomingAmt here.
1 participant
This PR is based on #10535.
Moves the native SQL payments schema migrations from the dev/test-only
migrations_dev.gointo the mainlinemigrationConfig, making themavailable in production builds.
When this PR is merged this can be closed: #9861