Process replication slot changes - #4930
Open
DilwoarH wants to merge 26 commits into
Open
Conversation
DilwoarH
force-pushed
the
process-replication-slot-changes
branch
6 times, most recently
from
August 11, 2026 09:53
6bddcf6 to
2d6790a
Compare
This will be replaced with the actual task in the following commits
the class is not processing all replication slot changes - just notifications
These are not needed
…eplication slot changes
Also include bst_date in the dimensions
DilwoarH
force-pushed
the
process-replication-slot-changes
branch
from
August 17, 2026 22:20
ed72e86 to
35de7dd
Compare
…t stats updates and slot advancement in a single transaction
…eplication slot accordingly
DilwoarH
force-pushed
the
process-replication-slot-changes
branch
from
August 18, 2026 00:15
da47acd to
1f988ae
Compare
Contributor
Author
|
requires #4936 to be merged first |
… advancement and update related tests
DilwoarH
force-pushed
the
process-replication-slot-changes
branch
from
August 18, 2026 00:29
1f988ae to
1d032b9
Compare
spatel033
reviewed
Aug 19, 2026
| "notification_type": dimensions["notification_type"], | ||
| "notification_status": dimensions["notification_status"], | ||
| } | ||
| filters = ( |
Contributor
There was a problem hiding this comment.
filters is used only in else block so better to move inside else block to avoid evaluating the SQLAlchemy expressions on increments
spatel033
reviewed
Aug 24, 2026
| change_counts: Counter[ServiceStatsDimensionsKey] = Counter() | ||
| for dimensions, change_count in counter.items(): | ||
| bst_date, template_id, service_id, notification_type, notification_status = dimensions | ||
| change_counts[(bst_date, service_id, template_id, notification_type, notification_status)] += change_count |
Contributor
There was a problem hiding this comment.
There is a confusing order swap between template_id and service_id, is this intentional?
spatel033
reviewed
Aug 24, 2026
| if key_type == "test": | ||
| return None | ||
|
|
||
| if require_status_from_primary_row and not notification_status: |
Contributor
There was a problem hiding this comment.
Do we actually need this condition check? Updated notifications always have a previous notification_status from the database, this condition seems redundant and a bit confusing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
** DO NOT MERGE UNTIL #4946 IS MERGED IN FIRST**
What
The application code for processing slot changes for notifications table
Why
This is needed to process WAL logs in the database
Test results
Ran a load test on
dev-c- this is the result after 2400 emails.How it works