-
Notifications
You must be signed in to change notification settings - Fork 975
plugins/sql: Add refresh time tracking, time_msec support, and extend pagination to all indexed tables #8841
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi! Thanks for this patch series; you've chosen an interesting rabbit hole to dig into! Since it's one your first contributions, I'm going to nitpick. In particular, you should never use merge; we rebase only. This keeps our tree linear and clean (at cost of some rebasing). Firstly, I apologize for the brevity of these "TODO": they were reminders to myself and not really for external consumption.
In particular, these tables (unlike chainmoves and channelmoves) can have entries deleted or changed. We need to spot that, and at the minimum, reload them. Fortunately, such deletions are usually done occasionally by autoclean, so we don't have to be clever here. We can simply use the wait API to ask if there have been any new deletions, and if so, reload the entire table. Handling changes is harder! We could do the same thing (reload if anything changes), but changes are more common. A first pass might be to implement this, with tests, to make sure that works. Then refine it: use the wait API async to monitor changes. Keep track of which records changed, and reload only those when we go to refresh. Note three complications:
Feel free to reach out if you have any questions! |
8011f41 to
239a1ab
Compare
Changelog-Added: Added sqlstatus command to monitor table refresh status Changelog-Added: Added time_msec field type support for timestamp fields Changelog-Changed: listsqlschemas now includes last_refresh_seconds_ago to show data freshness Changelog-Changed: listhtlcs, listforwards, listinvoices, and listsendpays now use pagination for better performance
Changelog-None
Changelog-None
Changelog-Added: SQL plugin now reloads mutable tables fully on change/delete events
79e8663 to
d7a5117
Compare
|
For the invoices, forwards, htlcs, and sendpays tables, a full reload is implemented for any event from the wait API (deletion or modification) to ensure data consistency, given that the wait API does not return change identifiers. If in the future the wait API returns created_index or change details, it will be possible to optimize and perform point updates. Is this what You had meant, or should i try a different approach, like trying to track changes asynchronously and only update the changed records? |
Changelog-None
Problem
The SQL plugin had several TODO items:
Solution
1. TODO 2: Refresh time tracking in API
Added tracking of table refresh completion times:
2. TODO 8: time_msec field type support
Added FIELD_TIME_MSEC enum type for millisecond timestamps:
3. TODO 10: General pagination API
Extended pagination from 2 tables to 6 tables using refresh_by_created_index:
About TODO 11: Normalize account_id fields.
The problem is - account_id in chainmoves and channelmoves tables is TEXT and highly duplicated (same values like "wallet", "external" repeated thousands of times).
I propose such solution:
That will give us:
Implementation may be like this:
Should this be automatic migration or optional flag?
Keep old column as deprecated or remove completely?
Changelog
Important
26.04 FREEZE March 11th: Non-bugfix PRs not ready by this date will wait for 26.06.
RC1 is scheduled on March 23rd
The final release is scheduled for April 15th.
Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:
tools/lightning-downgrade