invoices/sql_store: replace catch-all FilterInvoices with targeted index-friendly queries#10601
Conversation
Summary of ChangesHello @ziggie1984, 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 significantly enhances the performance of invoice querying by refactoring the database interaction layer. The previous 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
Activity
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 is a great performance improvement. It replaces a single, inefficient FilterInvoices SQL query with five specialized, index-friendly queries. This change is particularly beneficial for SQLite, as it avoids full table scans by using sargable predicates and direct ORDER BY clauses. The implementation is clean, well-structured, and the new functions are clearly documented. I have one minor suggestion to improve the clarity of a comment in one of the new SQL queries.
lightninglabs-deploy
left a comment
lightninglabs-deploy
left a comment
There was a problem hiding this comment.
Review: invoices/sql_store – replace catch-all FilterInvoices with targeted queries
Great PR – the problem description is excellent and the solution is well-structured. Replacing the col >= $param OR $param IS NULL anti-pattern and the dynamic CASE-based ORDER BY with focused queries is the right call for SQLite's planner. The add → migrate → remove commit structure is clean and keeps every commit compilable independently.
A few observations below:
1. Comment/SQL mismatch on FilterInvoicesByAddIndex
The SQL comment and the Go doc comment both say the query returns invoices whose id is "strictly greater than the given value", but the actual SQL predicate is WHERE id >= @add_index_get (greater-than-or-equal). The callers compensate by passing idx + 1, so the runtime behavior is correct, but the doc is misleading about the query's own semantics. I'd suggest updating the comment to say "greater than or equal to" to match the SQL, or noting that the caller is expected to adjust the bound.
(The Gemini bot flagged this too, so just confirming it's a real nit.)
2. SettleIndexGet remains sql.NullInt64
FilterInvoicesBySettleIndexParams.SettleIndexGet is typed as sql.NullInt64 even though the whole point of the refactor is to give the planner concrete, non-nullable values. This happens because settle_index is a nullable column (BIGINT without NOT NULL) and sqlc infers the parameter type from the column. Since the caller always passes a valid sql.NullInt64{Valid: true, ...}, this is functionally fine – but it's a minor inconsistency with the stated design goal. Not blocking, just worth being aware of.
3. NOT @pending_only and query plan implications
The NOT @pending_only OR state IN (0, 3) pattern is a clean replacement for the old CASE WHEN … THEN … END approach, and it works correctly across both SQLite (NOT 0 = 1, NOT 1 = 0) and PostgreSQL.
One thing to keep in mind: because $2 is a bound parameter, the planner still can't constant-fold NOT $2 at prepare time. So when pending_only = true, the planner won't be able to use invoices_state_idx for the state filter – it will rely on the primary-key index for the id >= $1 ORDER BY id scan and apply the state predicate as a post-scan filter. This is almost certainly fine for the QueryInvoices path (where the id range scan is the primary access pattern), but it's worth noting that the dedicated FetchPendingInvoices query is still the right choice when you specifically need state-indexed access.
4. Minor: sentinel defaults could be constants
The sentinel bounds (time.Unix(0, 0).UTC() and time.Date(9999, 12, 31, 23, 59, 59, 0, time.UTC)) are clear, but extracting them as package-level vars or const-like values would make them easier to reuse and document. The PR description mentions this follows "the same convention already established by the payments query" – but I wasn't able to find an equivalent pattern in the payments SQL code, so this might be the first place establishing the convention. A named constant would help future code follow the same pattern consistently.
Overall
This is a solid, well-motivated optimization. The behavioral equivalence looks correct across all five call sites (FetchPendingInvoices, InvoicesSettledSince, InvoicesAddedSince, QueryInvoices forward, and QueryInvoices reverse). The sentinel-default approach for timestamps is clean and avoids the nullable-parameter trap entirely for the forward/reverse paths.
Nice work 👍
| // settle_index is >= the given bound, ordered by id ascending. The | ||
| // caller must always supply a concrete lower bound so the planner can | ||
| // use invoices_settle_index_idx. | ||
| FilterInvoicesBySettleIndex(ctx context.Context, |
There was a problem hiding this comment.
We should add tests for these new variants.
There was a problem hiding this comment.
Done — all five new query variants now have corresponding tests in invoices/invoices_test.go:
FetchPendingInvoices→testFetchPendingInvoicesAccepted(verifiesContractOpenandContractAcceptedare returned,ContractSettledandContractCanceledare excluded)FilterInvoicesByAddIndex→testQueryInvoicesPaginationAddIndexFilterInvoicesBySettleIndex→testQueryInvoicesPaginationSettleIndexFilterInvoicesForward/FilterInvoicesReverse→testQueryInvoicesPagination(covers both directions)
ziggie1984
force-pushed
the
invoice-filter-index-optimization
branch
2 times, most recently
from
88361f9 to
412381c
Compare
The existing FilterInvoices query uses optional parameters via the
pattern `(col >= param OR param IS NULL)` for every filter. SQLite
cannot use indexes with this pattern because the OR prevents the query
planner from determining at plan time which rows satisfy the condition,
resulting in a full table scan regardless of the available indexes
(invoices_state_idx, invoices_settle_index_idx, and the primary-key
clustered index on id).
Additionally, the conditional ORDER BY:
ORDER BY CASE WHEN reverse = FALSE ... THEN id ELSE NULL END ASC,
CASE WHEN reverse = TRUE ... THEN id ELSE NULL END DESC
prevents the planner from using the index ordering and forces an
explicit sort.
Add five focused replacements, each with a plain sargable predicate and
a direct ORDER BY so the planner can always choose an index scan:
- FetchPendingInvoices: WHERE state IN (0, 3)
- FilterInvoicesBySettleIndex: WHERE settle_index >= $1
- FilterInvoicesByAddIndex: WHERE id >= $1
- FilterInvoicesForward: WHERE id >= $1 ... ORDER BY id ASC
- FilterInvoicesReverse: WHERE id <= $1 ... ORDER BY id DESC
FilterInvoicesForward and FilterInvoicesReverse accept non-nullable
timestamp parameters (created_after, created_before) so the planner
always sees plain range predicates on created_at. Callers supply
Go-side defaults when no date filter is needed, following the same
convention already used by the payments query.
FilterInvoices is kept in this commit so all existing call sites
continue to compile. It will be removed once all callers have been
migrated.
ziggie1984
force-pushed
the
invoice-filter-index-optimization
branch
from
412381c to
d2c4e73
Compare
Replace all four FilterInvoices call sites with the focused queries introduced in the previous commit: FetchPendingInvoices → FetchPendingInvoices InvoicesSettledSince → FilterInvoicesBySettleIndex InvoicesAddedSince → FilterInvoicesByAddIndex QueryInvoices → FilterInvoicesForward / FilterInvoicesReverse The first three are straight 1:1 swaps — the new params structs carry only the fields that are actually used, and the removed fields (Reverse, PendingOnly, unused index bounds) were always left at their zero values. QueryInvoices is restructured more substantially. The forward/reverse branch now selects between FilterInvoicesForward and FilterInvoicesReverse, each of which takes a concrete id bound that is always set: forward: AddIndexGet = IndexOffset + 1 (≥ 1 when IndexOffset = 0) reverse: AddIndexLet = IndexOffset - 1 (or MaxInt64 when offset = 0) Timestamp parameters are changed from nullable (sql.NullTime with OR-based SQL fallbacks) to always-on Go-side defaults, consistent with the approach used by the payments query: createdAfter → time.Unix(0, 0).UTC() (epoch, before any invoice) createdBefore → time.Date(9999, 12, 31, …) (far future, no upper cap) This ensures the planner always sees plain range predicates on created_at and can use the invoices_created_at_idx index. The SQLInvoiceQueries interface is updated to expose the five new methods and drop FilterInvoices.
All call sites have been migrated to the targeted replacement queries in the previous commit. Remove FilterInvoices and its associated params struct from the SQL source and regenerate.
Add testFetchPendingInvoicesAccepted to explicitly verify the state filtering behaviour of FetchPendingInvoices across all four contract states: ContractOpen (state 0) – must be returned ContractAccepted (state 3) – must be returned ContractSettled (state 1) – must be excluded ContractCanceled (state 2) – must be excluded This directly exercises the `state IN (0, 3)` predicate introduced in the FetchPendingInvoices SQL query and addresses the review request for explicit test coverage of the new targeted query variants. The test runs against the KV, SQLite, and Postgres backends.
ziggie1984
force-pushed
the
invoice-filter-index-optimization
branch
from
d2c4e73 to
1e5e6f2
Compare
|
I analyzed the new queries on a local SQLite instance using
|
3 participants
Problem
FilterInvoiceswas a single catch-all query that handled everylist/scan path through a set of optional parameters using the pattern:
This pattern is problematic for SQLite's query planner. When a
parameter is
NULLtheORbranch short-circuits toTRUEfor everyrow, which means the planner cannot determine at plan time which rows
satisfy the condition. As a result it falls back to a full table
scan even though suitable indexes exist (
invoices_state_idx,invoices_settle_index_idx, and the primary-key clustered index onid).The conditional
ORDER BYhad the same effect:A
CASEexpression inORDER BYprevents the planner from using indexordering, forcing an explicit sort pass over the full result set.
PostgreSQL's more sophisticated planner can sometimes work around these
patterns via constant-folding or partial index scans, but SQLite cannot,
so every call to
ListInvoices,FetchPendingInvoices,InvoicesSettledSince, andInvoicesAddedSinceresulted in a fulltable scan regardless of how many invoices were in the database.
Solution
Replace
FilterInvoiceswith five focused queries, each with a singlesargable predicate and a direct
ORDER BY:FetchPendingInvoicesWHERE state IN (0, 3)invoices_state_idxFilterInvoicesBySettleIndexWHERE settle_index >= $1invoices_settle_index_idxFilterInvoicesByAddIndexWHERE id >= $1FilterInvoicesForwardWHERE id >= $1 … ORDER BY id ASCFilterInvoicesReverseWHERE id <= $1 … ORDER BY id DESCFilterInvoicesForwardandFilterInvoicesReversereplace the generalQueryInvoicespath. They accept non-nullable timestamp parameters sothe planner always sees plain range predicates on
created_at. When thecaller sets no date filter, Go-side sentinel defaults are used
(
time.Unix(0,0)as the lower bound, year 9999 as the upper bound),following the same convention already established by the payments query.
This avoids
OR param IS NULLfallbacks entirely.The migration is done in three self-compiling commits following an
add → migrate → remove pattern so every commit in the stack builds
independently.