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

Stop considering generated column definition as being its default value #6199

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

allan-simon
Copy link
Contributor

Q A
Type bug/feature/improvement
Fixed issues #6198

Summary

The pg_get_expr(adbin, adrelid) expression in Postgresql's internal table pg_attrdef is used to know a column definition

for most column, if this returns something, it means this column's default value. So DBAL has been considering has ALWAYS meaning it contains its default.

However for generated columns, it contains the value definition and not its default value, so we change the selectTableColumns' query to correctly set the 'default' column to null for generated columns

It will help setting correctly the column's attributes which in turn will help generate correct schema diff.

@allan-simon
Copy link
Contributor Author

allan-simon commented Oct 18, 2023

I've tested manually on Postgresql 10 (with no support at all of GENERATED column, hence the feature detection code ) , Postgresql 13, 14, 15

For automated test, I've written one, but to be honest I haven't found a documentation on how to run them

src/Schema/PostgreSQLSchemaManager.php Outdated Show resolved Hide resolved
src/Schema/PostgreSQLSchemaManager.php Outdated Show resolved Hide resolved
src/Schema/PostgreSQLSchemaManager.php Outdated Show resolved Hide resolved
src/Schema/PostgreSQLSchemaManager.php Outdated Show resolved Hide resolved
src/Schema/PostgreSQLSchemaManager.php Outdated Show resolved Hide resolved
tests/Functional/Schema/PostgreSQLSchemaManagerTest.php Outdated Show resolved Hide resolved
src/Schema/PostgreSQLSchemaManager.php Outdated Show resolved Hide resolved
@allan-simon
Copy link
Contributor Author

Ok I've finally been able to setup the CI locally for postgresql and correct the coding style

so now I'm pretty sure all will be green :)

src/Schema/PostgreSQLSchemaManager.php Outdated Show resolved Hide resolved
@allan-simon
Copy link
Contributor Author

@derrabus I see there's still some changes requested in this PR, but normally I've adressed everything ? Is there's something you're waiting from me ?

(I don't mean to push you, if it's just a "I didn't have time yet to check it" , no worry I'll way 😊 , it's just I'm suddenly thinking "maybe he's the one waiting for me" ^^" )

@derrabus
Copy link
Member

No, I'm aware that the ball's in my court here. 😓

src/Platforms/PostgreSQLPlatform.php Outdated Show resolved Hide resolved
src/Platforms/PostgreSQL120Platform.php Outdated Show resolved Hide resolved
src/Platforms/PostgreSQL120Platform.php Outdated Show resolved Hide resolved
src/Schema/PostgreSQLSchemaManager.php Outdated Show resolved Hide resolved
tests/Functional/Schema/PostgreSQLSchemaManagerTest.php Outdated Show resolved Hide resolved
tests/Functional/Schema/PostgreSQLSchemaManagerTest.php Outdated Show resolved Hide resolved
@allan-simon allan-simon force-pushed the fix_6198 branch 2 times, most recently from 361a954 to b4d9b2f Compare November 13, 2023 23:01
@allan-simon
Copy link
Contributor Author

repushed (I forgot to run phpcs locally) and squashed, the previous CI was only failing on IBM DB2 test, but it seems to me it was a flaky test

@allan-simon
Copy link
Contributor Author

@derrabus the fail seems to be a flaky test , can you relaunch them ?

src/Platforms/PostgreSQL120Platform.php Outdated Show resolved Hide resolved
src/Platforms/PostgreSQL120Platform.php Outdated Show resolved Hide resolved
src/Schema/PostgreSQLSchemaManager.php Outdated Show resolved Hide resolved
src/Schema/PostgreSQLSchemaManager.php Outdated Show resolved Hide resolved
src/Schema/PostgreSQLSchemaManager.php Outdated Show resolved Hide resolved
src/Schema/PostgreSQLSchemaManager.php Outdated Show resolved Hide resolved
src/Schema/PostgreSQLSchemaManager.php Show resolved Hide resolved
…ing its default value

The `pg_get_expr(adbin, adrelid)` expression in Postgresql's internal table `pg_attrdef` is used to know a column definition

for most column, if this returns something, it means this column's default value. So DBAL has been considering has ALWAYS meaning it contains its default.

However for generated columns, it contains the value definition and not its default value, so we change the selectTableColumns' query to correctly set the 'default' column to `null` for generated columns

It will help setting correctly the column's attributes which in turn will help generate correct schema diff.
@greg0ire
Copy link
Member

So DBAL has been considering has ALWAYS meaning it contains its default

Is this sentence broken? I don't understand it.

@allan-simon
Copy link
Contributor Author

allan-simon commented Mar 30, 2024

So DBAL has been considering has ALWAYS meaning it contains its default

Is this sentence broken? I don't understand it.

yes , sorry

I meant

DBAL was interpreting "pg_get_expr(adbin, adrelid) returns something" as meaning "the value returned is the default value of the column " while it's a more generic "definition value" which can be the default value , but can also be other things (like the generated column expression )

I think the confusion was that postgresql uses the abbreviated form "pg_attrdef" and one could have interpreted that as "postgresql attribute default" whereas it actually means "postgresql attribute definition"

greg0ire
greg0ire previously approved these changes Mar 31, 2024
@allan-simon
Copy link
Contributor Author

@derrabus friendly reminder this PR exists , tell me if you need anything from me :) I can make myself available for a pair review call if you want.

@derrabus derrabus changed the base branch from 3.8.x to 3.9.x August 14, 2024 11:01
@derrabus derrabus dismissed greg0ire’s stale review August 14, 2024 11:01

The base branch was changed.

@derrabus derrabus added this to the 3.9.0 milestone Aug 14, 2024
@derrabus derrabus merged commit 5f43767 into doctrine:3.9.x Aug 14, 2024
90 checks passed
@allan-simon
Copy link
Contributor Author

thanks a lot !

derrabus added a commit to derrabus/dbal that referenced this pull request Aug 14, 2024
* 3.9.x:
  fix doctrine#6198:  stop considering generated column definition as being its default value (doctrine#6199)
  Fix condition on Ascii String for SQL Server (doctrine#6389)
  Add DBTypes that are missing for TypeMapping (doctrine#6463)
derrabus added a commit to derrabus/dbal that referenced this pull request Aug 14, 2024
* 3.9.x:
  fix doctrine#6198:  stop considering generated column definition as being its default value (doctrine#6199)
  Fix condition on Ascii String for SQL Server (doctrine#6389)
  Add DBTypes that are missing for TypeMapping (doctrine#6463)
derrabus added a commit to derrabus/dbal that referenced this pull request Aug 14, 2024
* 3.9.x:
  fix doctrine#6198:  stop considering generated column definition as being its default value (doctrine#6199)
  Fix condition on Ascii String for SQL Server (doctrine#6389)
  Add DBTypes that are missing for TypeMapping (doctrine#6463)
derrabus added a commit that referenced this pull request Aug 14, 2024
|      Q       |   A
|------------- | -----------
| Type         | documentation
| Fixed issues | Follows #6199

#### Summary

This PR documents the new platform class introduced in #6199.
@derrabus derrabus changed the title fix #6198: stop considering generated column definition as being its default value Stop considering generated column definition as being its default value Aug 14, 2024
derrabus added a commit that referenced this pull request Aug 15, 2024
* 4.1.x:
  Prepare 3.9.0 and 4.1.0 (#6492)
  Deprecate support for Postgres 10 and 11 (#6495)
  Document the new PostgreSQL120Platform (#6494)
  fix #6198:  stop considering generated column definition as being its default value (#6199)
  Fix condition on Ascii String for SQL Server (#6389)
  Add DBTypes that are missing for TypeMapping (#6463)
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