dbal icon indicating copy to clipboard operation
dbal copied to clipboard

fix #6198: stop considering generated column definition as being its default value

Open allan-simon opened this issue 2 years ago • 9 comments

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 avatar Oct 18 '23 23:10 allan-simon

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

allan-simon avatar Oct 18 '23 23:10 allan-simon

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 :)

allan-simon avatar Nov 04 '23 11:11 allan-simon

@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 :blush: , it's just I'm suddenly thinking "maybe he's the one waiting for me" ^^" )

allan-simon avatar Nov 12 '23 21:11 allan-simon

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

derrabus avatar Nov 13 '23 07:11 derrabus

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 avatar Nov 13 '23 23:11 allan-simon

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

allan-simon avatar Dec 03 '23 20:12 allan-simon

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

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

greg0ire avatar Mar 30 '24 13:03 greg0ire

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"

allan-simon avatar Mar 30 '24 15:03 allan-simon

@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.

allan-simon avatar Jun 12 '24 09:06 allan-simon

thanks a lot !

allan-simon avatar Aug 14 '24 11:08 allan-simon