-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[Part 3b|*] Implement Third Part for SQL Backend functions #10368
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
[Part 3b|*] Implement Third Part for SQL Backend functions #10368
Conversation
cb9cde9 to
072544e
Compare
072544e to
31e548f
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request implements the third and final part of the payments SQL backend functions, focusing on payment control and write operations. The changes are extensive and well-executed, including the implementation of functions for initializing, registering, settling, failing, and deleting payments. The database schema has been improved to better model the relationship between payments and payment intents, and performance is enhanced through efficient batching and lightweight queries.
I have two minor suggestions: one to fix a broken link and improve wording in the release notes, and another to address a style guide violation regarding spacing in a switch statement. Overall, this is a solid contribution to the SQL backend migration.
| * Implement third(final) Part of SQL backend [payment functions] | ||
| (https://github.com/lightningnetwork/lnd/pull/10368) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The release note entry has a broken link due to a newline, and the wording could be more descriptive and consistent with other entries in this section.
| * Implement third(final) Part of SQL backend [payment functions] | |
| (https://github.com/lightningnetwork/lnd/pull/10368) | |
| * Implement payment control functions for the [payments db SQL Backend](https://github.com/lightningnetwork/lnd/pull/10368) |
ellemouton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
solid!
payments/db/sql_store.go
Outdated
| func (s *SQLStore) computePaymentStatusFromDB(ctx context.Context, | ||
| db SQLQueries, dbPayment sqlc.PaymentAndIntent) (PaymentStatus, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think there is any reason to make this a receiver method? rather leave as is so that devs dont accidentally use the s.DB.
loadPaymentResolutions below doesnt use the receiver. so that can also be converted to a function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I had to put it back again, I realized I was not using ExecuteBatchQuery when fetching the resolutions, now we need the QueryConfig for this call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you were right, they are all not needed, added an extra commit where I remove most of the pointer receiver functions where not needed. Sorry for the back and forth here.
31e548f to
0aa1657
Compare
payments/db/sql_store.go
Outdated
| var numPayments int | ||
| ctx := context.TODO() | ||
|
|
||
| extractCursor := func( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can fit in one line
payments/db/sql_store.go
Outdated
|
|
||
| err := s.db.ExecTx(ctx, sqldb.WriteTxOpt(), func(db SQLQueries) error { | ||
| // collectFunc extracts the payment ID from each payment row. | ||
| collectFunc := func(row sqlc.FilterPaymentsRow) (int64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can fit in one line
payments/db/sql_store.go
Outdated
| // loadPaymentResolutions is a single-payment wrapper around | ||
| // loadPaymentResolutionsBatchData for convenience and to prevent duplicate | ||
| // queries. | ||
| func (s *SQLStore) loadPaymentResolutions(ctx context.Context, db SQLQueries, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we have duplicates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was more referring to duplicate queries because when fetching the attempts for one payment no batching is needed, but we wrap it in the batching call.
payments/db/sql_store.go
Outdated
| // loadPaymentResolutionsBatchData loads only HTLC resolution types for multiple | ||
| // payments. This is a lightweight alternative to loadPaymentsBatchData that's | ||
| // optimized for operations that only need to determine payment status. | ||
| func (s *SQLStore) loadPaymentResolutionsBatchData(ctx context.Context, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized all these helper methods are missing unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I added one unit test for computePaymentStatus because this method does more than just call a batchquery for example. But the other helpers will all be tested when switching the payment tests to db agnostic meaning they are run for all db types. Or do you mean some particular functions you would like to see tested ?
sqldb/sqlc/queries/payments.sql
Outdated
| pi.intent_payload | ||
| FROM payment_htlc_attempts ha | ||
| INNER JOIN payments p ON p.id = ha.payment_id | ||
| LEFT JOIN payment_intents pi ON pi.payment_id = p.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this query again, I think we are fetching duplicated data here. If a payment has 10 inflight attempts, for each inflight attempt, we will fetch the full payment data, which is redundant. A more efficient approach is, we first get a list of payment IDs which are inflight, then we fetch those payments. Not sure if the db query optimizer will help, tho i think its job is to find the fastest way to assemble the rows only?
Also I'm wondering if there's a way for us to stop scanning once we've found an inflight attempt? Sth like return a list of unique payment objects that have at least one active attempt, this way we don't need to iterate through all inflight attempts, given that as long as there is at least one that is active, we know this payment is inflight. @gemini-code-assist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've made a very astute observation regarding the FetchAllInflightAttempts query. You are correct that if a payment has multiple inflight attempts, the current SQL query will indeed return duplicated payment and payment intent data for each attempt row. This is a valid point about redundancy.
While the Go code in FetchInFlightPayments does mitigate this by using the processedPayments map to ensure that only unique MPPayment objects are ultimately constructed and returned (so we don't process the same payment data multiple times), the underlying database query still transfers this redundant data over the wire, which can be inefficient, especially for payments with many inflight attempts.
Furthermore, there's a double-fetching of HTLC attempt data: the FetchAllInflightAttempts query fetches details for inflight attempts, and then loadPaymentsBatchData (which is called for each unique payment) re-fetches all HTLC attempts (including resolved ones) for those payments using FetchHtlcAttemptsForPayments. This is another area for potential optimization.
To address these inefficiencies, a more optimized approach would involve:
-
Simplify
FetchAllInflightAttemptsquery: Modify theFetchAllInflightAttemptsSQL query to only return the minimal necessary information frompayment_htlc_attemptsto identify unique payments and facilitate pagination. This would typically beha.id,ha.attempt_index, andha.payment_id. -
Enhance
loadPaymentsBatchData: TheloadPaymentsBatchDatafunction would then be responsible for fetching the completePaymentandPaymentIntentdetails for the uniquepayment_ids it receives (perhaps using a new batch query forpaymentsandpayment_intentsby ID, asFetchPaymentsByIDswas removed). It would also continue to fetch all HTLC attempts (inflight and resolved) for those payments usingFetchHtlcAttemptsForPayments.
This refactoring would ensure that payment and intent data is fetched only once per unique payment, and HTLC attempt data is fetched once (by FetchHtlcAttemptsForPayments within loadPaymentsBatchData) for all attempts of a given payment, leading to a more efficient overall process by reducing redundant data transfer and database operations.
For the lines you highlighted, the immediate change to remove the payments and payment_intents joins and their columns would look like this:
| LEFT JOIN payment_intents pi ON pi.payment_id = p.id | |
| FROM payment_htlc_attempts ha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gemini-code-assist how about sth like,
-- name: FetchInflightPaymentIDs :many
-- Fetches distinct payment IDs that have at least one in-flight HTLC attempt.
-- This query avoids fetching redundant payment or intent data for payments
-- with multiple in-flight attempts.
SELECT DISTINCT ha.payment_id
FROM payment_htlc_attempts ha
WHERE NOT EXISTS (
SELECT 1 FROM payment_htlc_attempt_resolutions hr
WHERE hr.attempt_index = ha.attempt_index
)
ORDER BY ha.payment_id ASC
LIMIT $1 OFFSET $2;Then inside the FetchInFlightPayments, we first do db.FetchInflightPaymentIDs then we call s.loadPaymentsBatchData?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an excellent suggestion, @yyforyongyu! Your proposed FetchInflightPaymentIDs query directly addresses the redundancy issue we discussed by fetching only the distinct payment_ids that have inflight HTLC attempts. This is a very efficient approach.
To fully implement this optimization, the FetchInFlightPayments function would be refactored to:
- First, call your new
FetchInflightPaymentIDsquery to get a list of uniquepayment_ids. - Then, use these unique
payment_ids to calls.loadPaymentsBatchData(which would internally useFetchPaymentsByIDsor a similar batch query) to fetch the complete payment and payment intent details for each unique payment. - Finally, it would need to fetch the HTLC attempts for these unique payments (e.g., using
FetchHtlcAttemptsForPaymentsor a simplifiedFetchAllInflightAttemptsthat only returns attempt-specific data).
As a direct consequence of this new approach, the FetchAllInflightAttempts query (the one currently highlighted) would no longer need to join with the payments and payment_intents tables, nor select their columns. It would only need to return the columns from payment_htlc_attempts that are necessary for identifying attempts and serving as a cursor for pagination (e.g., id, attempt_index, payment_id, and other ha columns if needed for attempt-specific processing).
Here's how the FetchAllInflightAttempts query could be simplified to remove the redundant payment and intent data, assuming the new FetchInflightPaymentIDs query and subsequent batch loading of payment data are implemented:
| LEFT JOIN payment_intents pi ON pi.payment_id = p.id | |
| ha.first_hop_amount_msat, | |
| ha.route_total_time_lock, | |
| ha.route_total_amount, | |
| ha.route_source_key | |
| FROM payment_htlc_attempts ha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I think your approach is better, will implement, I just want to point out that we did not fetch redundant data because there was a deduplication logic in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gemini-code-assist keep in mind that the current loadPaymentsBatchData does not load the payment and intent data, that's why I joined this data when fetching the attempts in the first place, do you think having this redundant data attached to the attempt query is such a performance bottleneck, do you have some proposals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the crucial clarification, @ziggie1984. You are absolutely correct that loadPaymentsBatchData currently focuses on fetching HTLC attempts, hops, and custom records, and does not load the payment and payment_intent data. My apologies for that oversight in my previous response; it's vital to have the correct context.
To directly answer your question: yes, having redundant payment and payment_intent data attached to each payment_htlc_attempt row in the FetchAllInflightAttempts query is a performance bottleneck. While the Go code effectively deduplicates this data in memory, the database still performs the joins and transfers the full, duplicated data over the wire. For payments with many inflight attempts, this can lead to significantly increased network traffic, higher database load, and more memory consumption on the application side before deduplication occurs.
Given this, I propose a refined approach that combines the benefits of fetching distinct payment IDs with your existing batching framework:
-
Simplify
FetchAllInflightAttempts(as previously suggested): This query should be made as lightweight as possible. It should only select columns frompayment_htlc_attempts(includingpayment_id) and not join withpaymentsorpayment_intents. This query's primary role would be to efficiently identify all inflight HTLC attempts and their associatedpayment_ids. -
Introduce a new batch query for
paymentsandpayment_intents: You would need a new SQL query (e.g.,FetchPaymentsAndIntentsByIDs) that takes a slice ofpayment_ids and efficiently fetches the correspondingpaymentandpayment_intentrecords. This query would be similar to theFetchPaymentquery but designed for batch retrieval. -
Modify
FetchInFlightPaymentsorchestration:- The
queryFuncforExecuteCollectAndBatchWithSharedDataQuerywould call the simplifiedFetchAllInflightAttempts(from step 1). - The
collectFuncwould extract thepayment_idfrom each simplified attempt row. - The
batchDataFuncwould be enhanced:- It would first gather all unique
payment_ids from the current batch of attempts (using theprocessedPaymentsmap for deduplication). - Then, it would call your new
FetchPaymentsAndIntentsByIDsquery (from step 2) to fetch thepaymentandpayment_intentdata for these unique IDs. - It would also continue to call
s.loadPaymentsBatchDatato fetch the HTLC attempts, hops, and custom records for these uniquepayment_ids. - The
paymentsBatchDatastruct would need to be extended to hold the results from the newFetchPaymentsAndIntentsByIDsquery.
- It would first gather all unique
- The
processAttemptfunction would then combine the simplifiedFetchAllInflightAttemptsRowdata with thepaymentandpayment_intentdata (now available inbatchData) to construct the completeMPPaymentobject.
- The
This approach ensures that:
- Redundant
paymentandpayment_intentdata is no longer transferred for each HTLC attempt. - Payment and intent data is fetched only once per unique payment, in an efficient batch.
- The existing batching framework (
ExecuteCollectAndBatchWithSharedDataQuery) is leveraged effectively.
This refactoring would provide a significant performance improvement by reducing data transfer and database processing for FetchInFlightPayments.
Here's the suggested modification to the FetchAllInflightAttempts query to remove the redundant payment and intent data:
| LEFT JOIN payment_intents pi ON pi.payment_id = p.id | |
| ha.first_hop_amount_msat, | |
| ha.route_total_time_lock, | |
| ha.route_total_amount, | |
| ha.route_source_key | |
| FROM payment_htlc_attempts ha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gemini-code-assist I think this is the best of both worlds will implemennt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! I'm glad we could arrive at a solution that leverages the strengths of your existing batching framework while significantly improving efficiency by reducing redundant data transfer. This approach should provide a robust and performant way to handle inflight payments.
Feel free to reach out if you encounter any further questions during the implementation. I'm here to help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yyforyongyu let me know what you think about the new approach
47606dd to
01d9986
Compare
0aa1657 to
c5667fd
Compare
c5667fd to
c16999a
Compare
We remove the SQLStore from most of the helper functions. This also makes sure we do not accidentally create a new db tx but use the provided db SQLQueries parameter.
89f7e72 to
6b4f773
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request implements the final part of the SQL backend for payments. It includes a significant refactoring of data loading functions to improve performance through batching, and introduces new functions for failing payments and attempts, and for fetching in-flight payments. The changes are well-structured and the new implementations appear correct and efficient. I've found a couple of minor bugs in error messages and a style guide violation regarding missing comments. I've also suggested a small wording improvement in the release notes.
yyforyongyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok one last question about DeletePayments and this should be good to go!
payments/db/sql_store.go
Outdated
|
|
||
| // paymentsAggregate holds the full payment data when batch loading core | ||
| // payment data and all the related data for a payment. | ||
| type paymentsAggregate struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: fullPaymentData or paymentCompleteData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
went with paymentsCompleteData with payments instead of payment because it includes multiple payments
payments/db/sql_store.go
Outdated
| type paymentsBatchData struct { | ||
| // paymentsCoreData holds the core payment and intent data for a batch of | ||
| // payments. | ||
| type paymentsCoreData struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: paymentBaseData or paymentHeaders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
went with paymentsBaseData, also with the payments prefix
payments/db/sql_store.go
Outdated
| // This does not include the core payment and intent data which is fetched | ||
| // separately. It includes the additional data like attempts, hops, hop custom | ||
| // records, and route custom records. | ||
| type paymentsRelatedData struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: paymentDetailsData or paymentChildrenData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I go with paymentsDetailsData , payments because it contains more than 1 payment
payments/db/sql_store.go
Outdated
| NumLimit: limit, | ||
| // For now there are only BOLT 11 payment | ||
| // intents. | ||
| IntentType: sqldb.SQLInt16( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just occurs to me that we don't need to have this query? Even when we support bolt12 invoices, when the user calls delete payments i don't think we need to filter the intent type first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point removed the filter for the intent_type.
We rename this variable to paymentsDetailsData because we will also need to batch load the core payment and intent data in future commits and this renaming should make it clear that this does match payment related data but not the core data which is in the payment and in the intent table.
6b4f773 to
cb7ce98
Compare
We wrap the fetchPayment db call and catch the case where no errors are found in the db, where we now return the ErrPaymentNotInitiated error.
We take inspiration from the graph sql implementation and name the variables accordingly.
cb7ce98 to
56e2480
Compare
ziggie1984
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok will add a proper test for fetching InflightPayments in Part4
payments/db/sql_store.go
Outdated
| // This does not include the core payment and intent data which is fetched | ||
| // separately. It includes the additional data like attempts, hops, hop custom | ||
| // records, and route custom records. | ||
| type paymentsRelatedData struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I go with paymentsDetailsData , payments because it contains more than 1 payment
payments/db/sql_store.go
Outdated
| NumLimit: limit, | ||
| // For now there are only BOLT 11 payment | ||
| // intents. | ||
| IntentType: sqldb.SQLInt16( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point removed the filter for the intent_type.
payments/db/sql_store.go
Outdated
|
|
||
| // paymentsAggregate holds the full payment data when batch loading core | ||
| // payment data and all the related data for a payment. | ||
| type paymentsAggregate struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
went with paymentsCompleteData with payments instead of payment because it includes multiple payments
payments/db/sql_store.go
Outdated
| type paymentsBatchData struct { | ||
| // paymentsCoreData holds the core payment and intent data for a batch of | ||
| // payments. | ||
| type paymentsCoreData struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
went with paymentsBaseData, also with the payments prefix
ellemouton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ lgtm! very nice ✨
yyforyongyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢 🚢 🚢
fbfb184
into
lightningnetwork:elle-payment-sql-series-new
Implements the Third and Final Part of Payments SQL backend functions. Follows after
#10291