-
Notifications
You must be signed in to change notification settings - Fork 1
[KOL-67] PostgreSQL: resync on demand #496
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
Conversation
|
I had to do some contortions to get kolme-store to be able to trigger a Trigger, since Trigger is defined in the kolme crate, so kolme-store doesn't have direct access to it. The code could be simplified if Trigger were moved somewhere that kolme-store can use it directly. Would it make sense to move it to the |
|
Currently there can be spurious remote data notifications (and therefore resyncs) due to own-writes or if the pg_notify listener has to reconnect. This could be prevented in kolme-store by keeping track of the latest local write height and skipping the notification if it matches the new record's height payload in the pg_notify event. |
|
For testing, I modified the |
Oh, that's perfect! Let me rereview with an approval, I'd just commented before because I hadn't realized you'd already demonstrated this is working as expected. Nice! |
Deploying kolme with
|
| Latest commit: |
3a45135
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://90476af2.kolme.pages.dev |
| Branch Preview URL: | https://kol-67-postgres-resync-on-de.kolme.pages.dev |
|
Along with the other changes, I also put in place some logic to prevent most spurious new remote data notification by having the Postgres store keep track of the height of the last block it inserted, therefore being able to detect when it receives a pg_notify event for a local block and skipping the notification. |
packages/kolme-store/src/postgres.rs
Outdated
| } | ||
| Err(err) => { | ||
| tracing::error!("PostgreSQL insert block notification listener error: {err:?}"); | ||
| tokio::time::sleep(tokio::time::Duration::from_secs(2)).await; |
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 choice to sleep here could use a comment explaining motivation. It's a bit surprising to me to see it here TBH.
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.
If we were in a situation where recv() immediately returns with an error repeatedly, we don't want to be in a situtation where resync is repeated in a tight loop. I picked two seconds because that's what you had in six-sigma-kolme's resync_loop.
Taking another look at this does make me realize that this logic should move to kolme::init_remote_data_listener, both because this kind of decision should be closer to where the resync is called, and because the one second timeout there means this two second sleep here will never finish anyway.
25fbaac to
97f9a74
Compare
This uses a PostgreSQL trigger on the
blockstable to send apg_notifynotification whenever a record is added. A listener in kolme-store receives these notifications and triggers a Trigger. In kolme, a subscriber to that Trigger callsresync