Repository navigation
feat: LSN tracker persistence through replication slot confirmed flush position - #3462
Conversation
commit: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3462 +/- ##
===========================================
- Coverage 87.73% 67.48% -20.25%
===========================================
Files 3 182 +179
Lines 163 9689 +9526
Branches 39 360 +321
===========================================
+ Hits 143 6539 +6396
- Misses 19 3148 +3129
- Partials 1 2 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
magnetised
left a comment
magnetised
left a comment
There was a problem hiding this comment.
Love this. Great idea. Just think we should get rid of a lot of the stuff in shape cache - it's slightly performative now.
| query = """ | ||
| SELECT confirmed_flush_lsn | ||
| FROM pg_replication_slots | ||
| WHERE slot_name = #{Utils.quote_string(slot_name)} |
There was a problem hiding this comment.
nit: just as a double-checking mechanism, we could add AND NOT active to the where clause here to make sure we're not reading LSN from a slot that's used by another consumer. We already ensure exclusive access with an advisory lock but I just feel like between any two independent queries unexpected stuff may happen...
So if the slot is active, we'd get back 0 rows and that should be treated as a crash of the replication client.
There was a problem hiding this comment.
Or maybe I'm overthinking too much and crashing the client here would open up a way for another Electric to steal the thunder.... I don't know.
There was a problem hiding this comment.
I think that we should rely on the advisory lock since we do so in various places - if we're querying this it means that we fully intend on using this replication slot as well. If it ends up being already active then the replication client will fail when it attempts to start streaming anyway
Closes #3448
This is part of a larger piece of work described in #3442
The initial idea was to have a separate, periodic persister of the last processed LSN, but as I was working through the code I realised that we might not even need that.
Since now consumers are idempotent in the transactions they handle (see #3452), even the our global last processed LSN is slightly behind any given shape that is ok.
Instead of persisting it, we actually recover it from the replication slot's confirmed flush LSN.
This way, shape recovery does not have to read all shapes' last processed LSN during startup.
If we need stronger guarantees of the global processed lsn being the max, we can also attempt to set it everytime we lazily recover an existing shape (if the shape's max lsn is larger than the current max processed lsn). Happy to include this in this PR or a next one.