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

EZEE-3003: Increased ezpage_attributes.value size limit #74

Merged
merged 1 commit into from
Apr 2, 2021

Conversation

webhdx
Copy link
Contributor

@webhdx webhdx commented Apr 1, 2021

JIRA: https://issues.ibexa.co/browse/EZEE-3003

Description

This PR fixes issue where big Landing Page cannot be saved because size limit on ezpage_attribues.value column is exceeded. It usually happens on long Code Block or more advanced Schedule Block configurations.

The solution to that issue is changing column type to LONGTEXT which maxes out at 4GB. MEDIUMTEXT (16MB) could be a good choice but you never know what's going on in client databases... Postgres is not affected because TEXT type has no size limit there.

Upgrade guide

That change requires altering column configuration on current installations:

MySQL/MariaDB

ALTER TABLE ezpage_attributes MODIFY value LONGTEXT;

Postgres

No changes needed.

@webhdx webhdx requested review from a team April 1, 2021 11:56
@webhdx webhdx self-assigned this Apr 1, 2021
@webhdx
Copy link
Contributor Author

webhdx commented Apr 1, 2021

@ezsystems/documentation-team please note this PR is dedicated for 2.5 release which means we have to enclose the upgrade script in the documentation.

Copy link

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

LGTM, although I'd rather go for MEDIUMTEXT.

For two reasons:

  1. Columns of such width will cause the customer's database to fill up space extremely quickly in case of an error.
  2. Texts of that size will not fit into PHP's memory anyway in most cases - and even when they do, they will starve the server out of resources.

@webhdx
Copy link
Contributor Author

webhdx commented Apr 1, 2021

  1. It will only fill space when you actually start writing big strings to that table. It means you have to be absolutely sure about what are you doing. What kind of "errors" are you talking about? Can you imagine the situation where it writes 4GB because of our mistake? It's more relistic that it will only occur in 3rd party code.
  2. So that we should let PHP handle it. If there is a lower limit somewhere it should be handled on the other level. We should not limit the database because PHP has its limits.

4GB is better choice here because we have to choose between 16MB and 4GB. 16MB is still a lot but I can image someone trying to write base64 encoded files or very large JSONs in it which could easily exceed 17MB and still be well below PHP capacity. Of course this is an example of very bad architecture but sadly this kind of stuff happens on integrators' side and then it gets reported as a bug in our software.

Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

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

Tested on 2.5 and 3.2, on both PostgreSQL and MySQL using a clean installation and upgrading an existing one (on MySQL).

I was able to enter large fragments of text (https://pastebin.com/WEa8FWAr) into the Code block and the upgrade procedure did not brek the existing block attribute entries.

QA Approved.

@lserwatka
Copy link
Member

Thank you @mnocon ! Merging

@lserwatka lserwatka merged commit 9710f6a into 2.5 Apr 2, 2021
@lserwatka lserwatka deleted the ezpage_attribute_length branch April 2, 2021 10:36
@lserwatka
Copy link
Member

Could you merge it up?

@DominikaK DominikaK added Doc needed The changes require some documentation and removed Doc needed The changes require some documentation labels Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants