Fix bugs in Parser::DBI::PostgreSQL data_type, size, and default_value #155
Conversation
The postgres function pg_get_expr used in reading the default values for columns always returns a properly quoted DDL expression. This means that the default values written to $table->add_field should always be scalar-refs, unless some further step were taken to parse the DDL for the case of simple strings. As it was (for the last 17 years?!) the parsed default would be a string of SQL and if passed back to a producer it would attempt to quote that SQL as a string value and, in almost all cases, fail. This was even wrong in the unit tests: is( $f3->default_value, "'FOO'::text", 'Default value is "FOO"' ); When written back to the database, 'FOO'::text gets further wrapped in quotes like '''FOO''::text' resulting in the wrong default value. for any other data type, it simply fails to create thetable.
The handling of column sizes was also broken for the postgres parser. It was returning a data_type of 'character varying(50)' with a size of '54', when it should be returning a type of 'character varying' and size of '54'. When passed back to the producer, it generates syntax errors of "character varying(50)(54)". For numeric(8,4), it was ignoring the size entirely. For reasons undiscovered, the postgres column atttypmod does not contain useful numbers to represent the size, so I just parse the size from the data_type and then remove it. This works for at least varchar and numeric columns (as shown in the unit test). It might not be a universal solution but it is at least a lot more useful than the previous broken state of things. While I was at it, I added a check for default-values that are simple unquoted numbers or simple quoted strings, and un-quote them to be more useful to the cross-database 'translator' aspect of SQL::Translator.
The previous commit had broken handling of escaped single quote characters in a literal string default value. this fixes that and adds a unit test for it.
|
let me review this. |
|
@nrdvana this looks fine; i assume you've been using it happily since you wrote this PR? If so, i'll get it merged post-haste |
|
Yes, the patch is part of one of my projects and "works for me". I was just puzzled whether it could have been working for anyone else pre-patch (because it sure looks like it wouldn't). I'm not sure how to locate such users to ask them though. If you roll it out and someone complains, I'm happy to handle the discussion with them and take the blame :-) |
|
deal 😄 |
|
I never ran into any of these issues b/c i turn quoting off |
|
If by "quoting" you mean column name quoting, that would be related to #154 where quoting the names of the columns in an index prevents extra info from being appended to the column name. This one is about value quoting, where Postgres reading default values of columns get an extra layer of quotes around them. My only guess would be that nobody is both schema-loading and then |
I'm rather surprised to find bugs this large, so please review and see if I have any wrong assumptions here.
I'm running into cases where postgres round-trip from a DBI connection was generating a lot of syntax errors. It seems to have broken handling of column types with sizes, and broken handling of default values for anything other than numeric constants.
The changes are maybe best described by the changes to unit test 66. You can see that the parser used to store char column size in the data_type instead of ->size field, and have an incorrect value in the ->size field, and store literal DDL as plain strings for the default_value. I changed these to behave like other parsers and put the real size in the size column, and use scalar-refs for any default value that is literal DDL.