Skip to content

Use provided length for enum type if given for non-mysql platform #7034

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

Open
wants to merge 2 commits into
base: 4.4.x
Choose a base branch
from

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Jul 16, 2025

Q A
Type improvement ? fix ?
Fixed issues

Summary

When a Type Enum is used AND a length

#[ORM\Column(name: 'role', type: Types::Enum, length: 50)

the length is not used at all when generating the sql.

This makes sens for MysqlPlatform which list all the possible case, but for non-mysql platform it could be a way to reserved more length in advance in order to avoid futur ALTER TABLE.
The current strategy is too look at the max length of all the existing values, which means that adding new enum values might trigger every time an ALTER table just to use a bigger length.

@VincentLanglet VincentLanglet force-pushed the enumLength branch 2 times, most recently from 2523651 to f34ba6a Compare July 16, 2025 09:09
@morozov
Copy link
Member

morozov commented Jul 16, 2025

but for non-mysql platform it could be a way to reserved more length in advance in order to avoid futur ALTER TABLE.

What's the problem with future ALTER TABLE? On MySQL, if you add a new element to the enum, there will be an ALTER TABLE. Why avoid it on other platforms?

@VincentLanglet
Copy link
Contributor Author

What's the problem with future ALTER TABLE?

First, it feels odd to have

#[ORM\Column(name: 'role', type: Types::Enum, length: 50)

not throwing an error if the length is not used at all.

Then, running ALTER TABLE query might be slow if you already have something like million or billion of rows, no ?
It could be from Varchar(5) to VarChar(10) when adding an enum value or worst from VarChar(10) to VarChar(5) when removing one (And I assume the database will need to check every existing row length).
So how about allowing to avoid such extra query ?

If someone wants futur ALTER TABLE, he just have to not provide any length in the column.
If someone wants to reduce futur ALTER TABLE, that will be the purpose of the length column.

On MySQL, if you add a new element to the enum, there will be an ALTER TABLE. Why avoid it on other platforms?

Mysql use specific syntax ENUM() which really restrict the possible values we can save in the database, so there is a real benefit updating the table. For other Platform there is no such specific syntax and doctrine rely on string field ; so there is no benefit updating every time the column definition.

For example with Postgres, we can define constraint check

ALTER TABLE foo ADD CONSTRAINT foo_check CHECK (bar IN ('A', 'B', 'C'))

and when changing the enum, only the constraint is needed to be updated, but there is no gain to update the field length everytime (if we create the field with a bigger length first time).

@derrabus
Copy link
Member

First, it feels odd to have

#[ORM\Column(name: 'role', type: Types::Enum, length: 50)

not throwing an error if the length is not used at all.

It will also not throw if you assign a precision to that column. 🤷🏻‍♂️

Then, running ALTER TABLE query might be slow if you already have something like million or billion of rows, no ?

The column has variable length already, so the operation of changing the maximum column length itself is cheap. However, if the column is part of an index, that index might need to be rebuilt which can be expesinve.

It could be from Varchar(5) to VarChar(10) when adding an enum value or worst from VarChar(10) to VarChar(5) when removing one (And I assume the database will need to check every existing row length). So how about allowing to avoid such extra query ?

Yeah, allowing to specify the length of the varchar field is reasonable. I think, we should allow it.

@derrabus derrabus changed the base branch from 4.3.x to 4.4.x July 17, 2025 16:03
@VincentLanglet VincentLanglet requested a review from derrabus July 17, 2025 16:09
@VincentLanglet VincentLanglet force-pushed the enumLength branch 2 times, most recently from 89241ce to cceced6 Compare July 17, 2025 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants