[Part 3|*] Implement Second Part for SQL Backend functions #10291
Conversation
ziggie1984
changed the base branch from
master
to
elle-payment-sql-series-new
ziggie1984
changed the title
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-3
branch
2 times, most recently
from
80de0dd to
c0134ed
Compare
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-3
branch
3 times, most recently
from
f0e3302 to
99ddf07
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 a significant portion of the insert and delete methods for the new SQL backend for the payments database. The changes are extensive and include new SQL queries, data conversion functions, and the implementation of several methods on the SQLStore.
The use of ExecuteBatchQuery and ExecutePaginatedQuery is great for performance. The new PaymentAndIntent interface is a good abstraction.
I've found a few areas for improvement:
- Adherence to the function commenting style guide for exported functions.
- Consistent use of
context.Contextpropagation instead ofcontext.TODO(). - Opportunities for performance improvements by using batch inserts and optimizing queries to avoid extra fetches.
- A potential code smell related to handling of nil slices from the database.
Overall, this is a solid step forward in the SQL backend implementation. Addressing these points will improve the code's maintainability, robustness, and performance.
| func (s *SQLStore) FetchPayment(paymentHash lntypes.Hash) (*MPPayment, error) { | ||
| ctx := context.TODO() |
There was a problem hiding this comment.
This function should accept a context.Context from its caller instead of using context.TODO(). This allows for proper context propagation, which is crucial for handling cancellations and deadlines. This change will require updating the DB interface as well. Several other methods in this file (DeletePayment, DeleteFailedAttempts, InitPayment, RegisterAttempt, SettleAttempt) have the same issue and should also be updated.
| func (s *SQLStore) FetchPayment(paymentHash lntypes.Hash) (*MPPayment, error) { | |
| ctx := context.TODO() | |
| func (s *SQLStore) FetchPayment(ctx context.Context, paymentHash lntypes.Hash) (*MPPayment, error) { | |
| // TODO(ziggie): Can we get rid of this? | ||
| // This has to be in place otherwise the | ||
| // comparison will not match. | ||
| if record.Value == nil { | ||
| record.Value = []byte{} | ||
| } |
There was a problem hiding this comment.
The TODO here highlights a potential code smell. This workaround of converting a nil slice to an empty slice might be hiding an issue or could be simplified. It would be beneficial to investigate whether the downstream code that consumes these records can be made to handle nil slices gracefully. This would remove the need for this conversion and make the data handling more consistent.
| // QueryPayments queries the payments from the database. | ||
| // | ||
| // This is part of the DB interface. | ||
| func (s *SQLStore) QueryPayments(ctx context.Context, |
There was a problem hiding this comment.
The comment for this exported function does not fully adhere to the repository's style guide.1 The style guide requires that function comments begin with the function name and provide detailed information for the caller. Please update this comment to start with QueryPayments and add more details about its behavior, parameters, and return values. This applies to other new exported functions in this file as well, such as FetchPayment, DeletePayment, etc.
Style Guide References
Footnotes
payments/db/sql_store.go
Outdated
| // Get the payment ID from the inserted payment | ||
| // We need to fetch the payment we just inserted to get its ID | ||
| insertedPayment, err := db.FetchPayment(ctx, paymentHash[:]) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to fetch inserted payment: %w", err) | ||
| } |
There was a problem hiding this comment.
To improve efficiency, this extra FetchPayment call can be avoided. The InsertPayment SQL query can be modified to return the ID of the newly inserted row. You can achieve this by changing the query in sqldb/sqlc/queries/payments.sql from :exec to :one and adding RETURNING id. The Go code would then be simplified to use the returned ID directly.
| func (s *SQLStore) insertRouteHops(ctx context.Context, db SQLQueries, | ||
| hops []*route.Hop, attemptID uint64) error { |
There was a problem hiding this comment.
This function inserts route hops and their associated data one by one in a loop. For routes with many hops, this can result in a large number of database queries, impacting performance. Consider refactoring this to use batch inserts. sqlc supports batch inserts, which would require modifying the SQL queries to accept slices of parameters and then calling them once with all the data for the hops.
In this commit we add all queries which we will need to insert payment related data into the db.
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-3
branch
from
99ddf07 to
6145682
Compare
1 participant
Depends on #10287
This PR implements most of the Insert and Delete methods we need for the unit tests in the following PR.