-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: Add migration in advance of session properties PR #21703
feat: Add migration in advance of session properties PR #21703
Conversation
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.
without looking at the parent PR so not commenting on the why
we don't have a good way of testing/predicting what impact changes like this have on production 🙈
locally this runs and I can still filter properties afterwards
(to state the obvious - the alternative to avoid the DB migration is to have checks like this in the code (with the associated downsides of not having safety in the DB))
migrations.AlterField( | ||
model_name="propertydefinition", | ||
name="property_type", | ||
field=models.CharField( | ||
blank=True, | ||
choices=[ | ||
("DateTime", "DateTime"), | ||
("String", "String"), | ||
("Numeric", "Numeric"), | ||
("Boolean", "Boolean"), | ||
("Duration", "Duration"), | ||
], | ||
max_length=50, | ||
null=True, | ||
), | ||
), | ||
migrations.AlterField( | ||
model_name="propertydefinition", | ||
name="type", | ||
field=models.PositiveSmallIntegerField( | ||
choices=[(1, "event"), (2, "person"), (3, "group"), (4, "session")], default=1 | ||
), | ||
), |
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.
IIRC these are not really database changes so they have no effect on the DB
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.e. Django lists them as migrations but they are no-op for postgres)
] | ||
|
||
operations = [ | ||
migrations.RemoveConstraint( |
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.
maybe this is the only written up previous incident from migrations
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 the drawback we have is even a canary deploy doesn't help us since it's the DB that we lock if something does go wrong
but I also don't think we have a good alternative here, this is what Django is going to do to make this change 🙈
probably we need to merge this when the system isn't under load and with someone from infra on hand ready to help fix all the things if it does cause downtime.
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.
(and feel happily foolish if it goes in without incident :))
Django 4 supports AddConstraintNoValid which would be perfect here - you can add a constraint without checking that existing rows are valid, which is obviously correct when adding a new enum - sad we can't use that yet |
curious why you can't use it yet? Also, I faced this before too, when we weren't on django 4.2, and I went this route to make it work -> https://github.com/PostHog/posthog/blob/master/posthog/migrations/0365_update_created_by_flag_constraint.py#L24-L27 |
(and follow with a migration to validate the constraint) - could you check how big the property definition table is on prod? |
c9df14b
to
ccf43f9
Compare
ccf43f9
to
549b311
Compare
@neilkakkar I'm being silly - I can use it. I've changed this PR to use it, and part 2 will do the validation Part 2: #21777
|
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.
Looks good to me! This extra effort with not valid constraints is indeed the way to go here - table + indexes is ~32GB so blocking can take more than a minute
SELECT pg_size_pretty(pg_total_relation_size('posthog_propertydefinition'));
@@ -366,6 +366,7 @@ posthog/session_recordings/queries/session_recording_list_from_replay_summary.py | |||
posthog/session_recordings/queries/session_recording_list_from_replay_summary.py:0: note: If the method is meant to be abstract, use @abc.abstractmethod | |||
posthog/session_recordings/queries/session_recording_list_from_replay_summary.py:0: error: Missing return statement [empty-body] | |||
posthog/session_recordings/queries/session_recording_list_from_replay_summary.py:0: note: If the method is meant to be abstract, use @abc.abstractmethod | |||
posthog/migrations/0404_remove_propertydefinition_property_type_is_valid_and_more.py:0: error: Module "django.contrib.postgres.operations" has no attribute "AddConstraintNotValid" [attr-defined] |
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.
are types borked here?
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.
Yeah, it does run locally though
Problem
Pulled out of #21512 - doesn't affect anything else and makes the large PR easier to merge if we're not also doing a DB migration
select count(*) from posthog_propertydefinition
Changes
Support Duration properies (we already support these, but never server-side, they are just hard-coded client side).
Support session properties (also were previously client-side only)
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Existing tests pass