Skip to content
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

Increase Element model varchar limits #3912

Merged
merged 1 commit into from Apr 8, 2021
Merged

Conversation

Twixes
Copy link
Collaborator

@Twixes Twixes commented Apr 8, 2021

Changes

Model field change to support larger elements in Postgres PostHog and in ClickHouse webhooks (which do matching in Postgres). Resolves #3913.

@Twixes Twixes requested a review from paolodamico April 8, 2021 14:21
@timgl timgl temporarily deployed to posthog-pr-3912 April 8, 2021 14:25 Inactive
Copy link
Contributor

@paolodamico paolodamico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

I believe CharField & TextField have no significant differences with a Postgres backend, so just a nit, but maybe TextField would better reflect these attributes?

@macobo
Copy link
Contributor

macobo commented Apr 8, 2021

This operation WILL cause issues both in prod and for self-hosted.

ALTER TABLE on huge tables is insanely expensive since it needs to rewrite the whole table.

@Twixes
Copy link
Collaborator Author

Twixes commented Apr 8, 2021

@paolodamico
We could, but I think there is a size we should not cooperate with, so I'd leave it some limit in. Ridiculously big elements could cause performance problems.

@macobo
This is not the case for this type of operation since Postgres 9.2 @macobo.
From https://www.postgresql.org/docs/9.2/release-9-2.html#AEN114949:

Increasing the length limit for a varchar or varbit column, or removing the limit altogether, no longer requires a table rewrite.

@macobo
Copy link
Contributor

macobo commented Apr 8, 2021

You're correct - these should be instantaneous assuming no long-running queries holding locks block them.

posthog> select count(*) from posthog_element;
+----------+
| count    |
|----------|
| 26525865 |
+----------+
posthog> ALTER TABLE posthog_element ALTER COLUMN text TYPE VARCHAR(1000);
ALTER TABLE
Time: 0.166s

@Twixes Twixes merged commit 305e5f7 into master Apr 8, 2021
@Twixes Twixes deleted the increase-element-varchars branch April 8, 2021 18:17
Twixes added a commit that referenced this pull request Apr 8, 2021
Twixes added a commit that referenced this pull request Apr 8, 2021
fuziontech pushed a commit that referenced this pull request Apr 19, 2021
…o 3765-cohort-by-trend

* '3765-cohort-by-trend' of github.com:PostHog/posthog: (39 commits)
  'string, parsable as datetime' (#3942)
  Update plugin server to 0.16.3 (#3944)
  Resizable table columns in Sessions (#3927)
  bump cryptography==3.4.7 and add macosx_arm64 install script (#3935)
  🤖: Add jeduden as a contributor 🎉 (#3938)
  Fix feature flags default rollout (#3745)
  Less dancing in dashboards (#3824)
  Always show event stats and add warnings (#3908)
  Update plugin server to 0.16.2 (#3932)
  Minimum PostHog version in plugins (#3916)
  Renames Active users to Unique users (#3930)
  Fix action with same name (#3909)
  Fix navigation to insights from dashboards (#3928)
  User V2 Part II - Frontend changes (#3866)
  update autocapture label to be more descriptive (#3925)
  Log to sentry when migrations are out of date (#3924)
  Revert "Increase Element model varchar limits (#3912)" (#3923)
  Update plugin server to 0.16.1 (#3920)
  Are migrations safe to run on cloud?  (#3917)
  Run Automerge as posthog-bot (#3919)
  ...
Twixes added a commit that referenced this pull request May 6, 2021
* Revert "Revert "Increase Element model varchar limits (#3912)" (#3923)"

This reverts commit e30681b.

* Use RunSQL to avoid ORM crap

* Fix attr_class being converted from varchar[] to varchar

* Bump migration

* Simplify migration and remove attr_class alter due to locking

* Bump migration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Events with large HTML elements don't work in Postgres
4 participants