-
-
Notifications
You must be signed in to change notification settings - Fork 730
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
fix: partial index on events announced #4856
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
.whereNotNull('announced') | ||
.where('created_at', '>', subDays(Date.now(), 1)) | ||
.returning(EVENT_COLUMNS) | ||
.limit(500); |
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.
Note: I've tested that limit does not work with update. There are alternatives but may require using native queries:
@@ -412,11 +411,7 @@ class EventStore implements IEventStore { | |||
const rows = await this.db(TABLE) | |||
.update({ announced: true }) | |||
.where('announced', false) | |||
.whereNotNull('announced') |
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.
Where not null no longer needed due to the migration
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.
I would wait for someone who can actually sanity check the SQL properly but yeah this does LGTM
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.
This should work, but can be heavy for instances with millions of events already in place.
To clarify, I'm not worried about the index, I'm pretty sure that will be fine. I'm a bit worried with the initial setup, setting all NULL announced to false, though I do think we mostly have remembered to set it, so the impact should be minimal. |
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.
Adding a thumbsup
We have DEFAULT false, but it was still nullable. I guess this was during a migration, but with the job marking unannounced events (both false or null) to true, we should have zero unannounced set to null. But I'm just doubling down on DEFAULT false just in case |
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.
LG
); | ||
}; | ||
|
||
exports.down = function (db, callback) { |
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.
I think a down function would be beneficial.
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.
I considered that but down migrations are not executed automatically, so if we want to make changes we're probably going to move forward rather than backward
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.
@sjaanus I know we're in a weird place with the down migrations and what we should do with them. I've never used them though. Are you using them? I don't remember any recommendations to users to consider down migrations either, our response has always been get latest version where we fixed the issue
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.
@sighphyre I have not used, but I know customers have backed down to previous versions and in some cases if we do not provide down migration, it may make their instance unusable.
For example, if we currently do not allow nulls and previously somehow they were entered, we would probably just have errors in logs, but in other cases, if we change something related to projects/strategies/segments, it might break UI.
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.
Oh yeah, if there are actually folks using them then it seems important
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.
I know customers have backed down to previous versions and in some cases if we do not provide down migration, it may make their instance unusable.
Indeed, but even in these cases, there's no command for the customer to execute the down migration other than by hand. They'd have to either go one by one, or identify the one they need to execute.
In such a scenario, the down migration does not update the migrations table (because we're not using the tool for this), and therefore the framework doing the up migration will still believe that the migration is applied and will never try to apply it again... this leaves the customer in a very weird situation.
I know because we faced this same problem with our demo instance and we learned that down migrations should not be applied manually because they don't include the change in the migrations table (and they shouldn't! The right solution is to use the tool's command for doing it)
## About the changes Add partial index on events by announced. This should help avoid `Seq Scan on events` when the majority of events are announced=true --- Co-authored-by: Ivar Østhus <[email protected]> Co-authored-by: Gard Rimestad <[email protected]>
About the changes
Add partial index on events by announced. This should help avoid
Seq Scan on events
when the majority of events are announced=trueCo-authored-by: Ivar Østhus [email protected]
Co-authored-by: Gard Rimestad [email protected]