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

Restrict some tables' columns size #14075

Closed
wants to merge 4 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Dec 20, 2020

As title.

UPDATE 1:

The default size of a string in XORM is 255, for most things, it's OK.
But for an index, the size will affect the performance.
According this PR, Type on reaction table is an index and will be +1, like and etc. 32 chars is enough for that.

OriginalAuthor, LoginName, Name is related with User, in fact, login name have been restricted on module/auth/user_form.go which is 40 chars. So no reason to use a column with 255 to store that.

@lunny lunny added the type/enhancement An improvement of existing functionality label Dec 20, 2020
models/issue_reaction.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 20, 2020
@CirnoT
Copy link
Contributor

CirnoT commented Dec 20, 2020

Please provide reasoning for why this PR is necessary and why only these specific columns were changed.

@lunny lunny force-pushed the lunny/reaction_col_size branch from 6e106c6 to d2f008e Compare December 21, 2020 11:26
@lunny
Copy link
Member Author

lunny commented Dec 21, 2020

Please provide reasoning for why this PR is necessary and why only these specific columns were changed.

I have updated the issue content and explain the reason there.

Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

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

Also conflicts

models/migrations/v159.go Outdated Show resolved Hide resolved
@lunny lunny added the pr/wip This PR is not ready for review label Jan 22, 2021
Type string `xorm:"VARCHAR(32) INDEX NOT NULL"`
OriginalAuthor string `xorm:"VARCHAR(64) INDEX"`
}
if err := x.Sync2(new(Reaction)); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

This will not work. Sync2 will only modify columns from small length to big length but not opposite.

@lunny lunny force-pushed the lunny/reaction_col_size branch from d2f008e to f1a8613 Compare January 26, 2021 12:56
@lunny
Copy link
Member Author

lunny commented Dec 18, 2022

Close as too many conflicts.

@lunny lunny closed this Dec 18, 2022
@lunny lunny deleted the lunny/reaction_col_size branch December 18, 2022 03:40
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. pr/wip This PR is not ready for review type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants