[Part 2|*] Implement First Part for SQL Backend functions #10287
Conversation
ziggie1984
changed the base branch from
master
to
elle-payment-sql-series-new
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-2
branch
2 times, most recently
from
fe51267 to
63aa866
Compare
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-2
branch
from
938443e to
b0361a2
Compare
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-2
branch
from
b0361a2 to
ed283fa
Compare
ziggie1984
changed the title
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-2
branch
from
ed283fa to
aa560da
Compare
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-2
branch
from
aa560da to
08464ea
Compare
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-2
branch
from
08464ea to
da3603e
Compare
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-2
branch
from
da3603e to
069f7c6
Compare
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-2
branch
2 times, most recently
from
1f7d395 to
ef6ef5c
Compare
|
/gemini review |
![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 implements the QueryPayments and FetchPayment functions for the new SQL backend of the payments database. The changes include new SQL queries, Go functions to execute them, and converters to transform database results into application-level types.
The implementation is well-structured, making good use of batching for efficient data fetching. I have a few suggestions to improve code clarity and adhere to best practices:
- Simplify time value handling in
sql_converters.goby using direct type assertions where the type is guaranteed. - Consider updating the
DBinterface to accept acontext.ContextinFetchPaymentto avoid usingcontext.TODO(). - I also found a minor typo in a comment.
Overall, this is a solid contribution towards the SQL backend migration.
| // | ||
| // This is part of the DB interface. | ||
| func (s *SQLStore) FetchPayment(paymentHash lntypes.Hash) (*MPPayment, error) { | ||
| ctx := context.TODO() |
There was a problem hiding this comment.
The use of context.TODO() is not ideal. The FetchPayment method in the DB interface should probably accept a context.Context to allow for cancellation and deadlines to be propagated. This would be a good improvement for a follow-up PR to align it with QueryPayments and other context-aware functions.
There was a problem hiding this comment.
will be done in a separate PR
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-2
branch
from
ef6ef5c to
0849296
Compare
| WHERE h.htlc_attempt_index IN (sqlc.slice('htlc_attempt_indices')/*SLICE:htlc_attempt_indices*/); | ||
|
|
||
| -- name: FetchPaymentLevelFirstHopCustomRecords :many | ||
| SELECT | ||
| l.id, | ||
| l.payment_id, | ||
| l.key, | ||
| l.value | ||
| FROM payment_first_hop_custom_records l | ||
| WHERE l.payment_id = $1; | ||
|
|
||
| -- name: FetchRouteLevelFirstHopCustomRecords :many | ||
| SELECT | ||
| l.id, | ||
| l.htlc_attempt_index, | ||
| l.key, | ||
| l.value | ||
| FROM payment_attempt_first_hop_custom_records l | ||
| WHERE l.htlc_attempt_index IN (sqlc.slice('htlc_attempt_indices')/*SLICE:htlc_attempt_indices*/); | ||
|
|
||
| -- name: FetchHopLevelCustomRecords :many | ||
| SELECT | ||
| l.id, | ||
| l.hop_id, | ||
| l.key, | ||
| l.value | ||
| FROM payment_hop_custom_records l |
There was a problem hiding this comment.
just a reminder that when we use these, we must always wrap them with the sqdb.ExecuteBatch helper so that we never overflow
There was a problem hiding this comment.
oh cool, i see you do use them 👍 noice!
| }, func() { | ||
| }) |
|
|
||
| // fetchHTLCAttemptsForPayment fetches all HTLC attempts for a payment and | ||
| // uses ExecuteBatchQuery to efficiently fetch hops and custom records. | ||
| func (s *SQLStore) fetchHTLCAttemptsForPayment(ctx context.Context, |
There was a problem hiding this comment.
ah, you can probably come back to this at the performance improvement stage like i did for graph stuff, but i think you can improve performance here quite a bit still: currently it is still only doing payment by payment. So it isnt taking full advantage of the batch query helper. You could instead batch fetch the components for a whole range of payment IDs in one go
2 participants
Depends on #9147
This PR implements the QueryPayments and FetchPayment SQL Payments DB backend functions.
It does not yet add testing for these functions which will be added in followup PRs once we have the ability to add payments and attempts to the database which we then can fetch.