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.
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.
Where not null no longer needed due to the migration
|
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. |
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 |
| ); | ||
| }; | ||
|
|
||
| exports.down = function (db, callback) { |
There was a problem hiding this comment.
I think a down function would be beneficial.
There was a problem hiding this comment.
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.
@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.
@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.
Oh yeah, if there are actually folks using them then it seems important
There was a problem hiding this comment.
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 <ivar@getunleash.io> Co-authored-by: Gard Rimestad <gard@getunleash.io>
About the changes
Add partial index on events by announced. This should help avoid
Seq Scan on eventswhen the majority of events are announced=trueCo-authored-by: Ivar Østhus ivar@getunleash.io
Co-authored-by: Gard Rimestad gard@getunleash.io