pgsql driver does not handle unsigned numeric fields
bjaspan - May 8, 2008 - 00:47
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | database system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | postponed |
Description
The D6 pgsql driver does not properly created unsigned numeric fields if the schema so specifies. I've attached a patch for D6. It applies to D7 as well. However, it also needs to be applied to the PDO changes in progress, so this issue should not be closed until it is in all three places.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| pgsql-numeric-unsigned.patch | 1.6 KB | Idle | Failed: Failed to apply patch. | View details | Re-test |
#1
I assume the changes to bootstrap.inc are "optional"? ;-)
Could you add a one-line code comment?
CHECK($name >= 0)is new to me.#2
The change to bootstrap is an accident. :-)
#3
Some discussion about this already occurred some years ago, in #60871.
#4
@#3: Thanks for the pointer, I did not know about that issue.
New patch attached. Turns out D6 pgsql driver also does not support unsigned floats; also fixed.
#5
I've committed this patch to CVS HEAD. Would be better if it came with tests.
#6
Okay, I'll mark it CNW pending tests.
#7
Tests for signed/unsigned int, float, and numeric columns for HEAD. Remember to 'cvs add' this new file.
#8
On 7.x, this patch causes a lot of exceptions during testing:
Unexpected PHP error [pg_query() [function.pg-query]: Query failed: ERROR: type "int_unsigned" already exists] severity [E_WARNING] in [/var/www/Drupal/Postgres-Head/includes/database.pgsql.inc line 138] [PHP] ExceptionUnexpected PHP error [ERROR: type "int_unsigned" already exists query: CREATE DOMAIN int_unsigned integer CHECK (VALUE >= 0)] severity [E_USER_WARNING] in [/var/www/Drupal/Postgres-Head/includes/database.pgsql.inc line 159] [PHP] Exception
Unexpected PHP error [pg_query() [function.pg-query]: Query failed: ERROR: type "smallint_unsigned" already exists] severity [E_WARNING] in [/var/www/Drupal/Postgres-Head/includes/database.pgsql.inc line 138] [PHP] Exception
Unexpected PHP error [ERROR: type "smallint_unsigned" already exists query: CREATE DOMAIN smallint_unsigned smallint CHECK (VALUE >= 0)] severity [E_USER_WARNING] in [/var/www/Drupal/Postgres-Head/includes/database.pgsql.inc line 159] [PHP] Exception
Unexpected PHP error [pg_query() [function.pg-query]: Query failed: ERROR: type "bigint_unsigned" already exists] severity [E_WARNING] in [/var/www/Drupal/Postgres-Head/includes/database.pgsql.inc line 138] [PHP] Exception
Unexpected PHP error [ERROR: type "bigint_unsigned" already exists query: CREATE DOMAIN bigint_unsigned bigint CHECK (VALUE >= 0)] severity [E_USER_WARNING] in [/var/www/Drupal/Postgres-Head/includes/database.pgsql.inc line 159] [PHP] Exception
I'm not sure if this is a simpletest / SchemaAPI bug or a bug with that patch.
#9
It is not this patch that causes those exceptions; every test on pgsql throws those exceptions with or without this patch.
See http://drupal.org/node/257009 which fixes simpletest for pgsql.
#10
Can I ask how this is coming along? I just ran into the same problem installing the development version of Ubercart with D6.4. The fix looks trivial, is there some holdup?
#11
I am having problems determining what the actual problem here is. The errors listed on the page are obvious, it can't create the type because the type already exists. What is the actual problem?
#12
If this works for 6.x, I'd ask that it be implemented, and that development to fix the issues in 7.x be continued separately.
A lot of people are using 6.x now, and this is bug prohibits some of us from using certain modules (for example, the Amazon module).
#13
The last submitted patch failed testing.
#14
Were duplicates:
#334791: PostgreSQL not handling unsigned properly
#334794: PostgreSQL not handling unsigned properly
#334598: PostgreSQL not handling unsigned properly
#15
I repeat the assertion tha this is a no-op problem. The errors that people mentioned are because the types already existed and it looks as if it has been solved in other tickets. I am going to close this ticket. Please re-open with an actual problem if there is one :)
#16
#17
In D7, DBTNG apparently fixes this by adding in CHECK constraints instead of the D5/D6 solution of creating domains.
<?php
protected function createFieldSql($name, $spec) {
...
if (!empty($spec['unsigned'])) {
$sql .= " CHECK ($name >= 0)";
}
...
return $sql;
}
?>
D5/D6 uses domains for the int types, but this won't work for numeric, since the precision and scale parameters need to be defined when the domain is created. I'd support back-porting the D7 solution as with schema api there isn't really any worry of breaking extant modules. The serial type is already handled this way.
The patch attached just changes the PGSQL driver function that generates the SQL for field definitions. It could be extended to take out the domain creation in system_install, although you would not want to drop the domains.
#18
The latest offender here is the Twitter module, which just changed it's primary key to numeric from int. On Postgres, this severely borks your table on update.
Also, please use this patch. I accidentally did not revert some other edits I was working on.
#19
Better yet, use this one. I had to move the unsigned bit after the length/precision bit. Going to file a new issue for D7, since the same error exists there.
#20
Might help if I included the patch.
#21
D7 patch here: #522786: Postgres numeric check needs to happen after precision check in schema generation
#22
Unsigned types are added to PostgreSQL during installation. The problem is that some are missing. Thanks to ccurvey for pointing this out in issue 359764.
#23
As a PG lurker, let me thank you all for doing this. It's been a constant source of frustration (and a source of secret shame to me when I hack install files and the odd function or two)...
#24
Darren, you can't specify precision, length, or scale for domains after they are created, ie,
ALTER TABLE foo ADD COLUMN bar numeric_unsigned (20,0);Will barf.
#25
Thanks.
#26
Now we're back to this issue's original post - the unsigned numeric type (specified in schema api) can't be created.
Also, I still think domains are an anti-pattern we don't need to use, and in fact are not used in D7. I presume the argument for them in D4 and D5 was to offer types that made it easier for people to just copy over mySql schema definitions, rather than using the CHECK clause in the column definition directly. I think we should just check for the unsigned flag on the appropriate types and add the CHECK clause rather than adding a bunch of domains and special casing things.
#27
As a side point, the use of a CHECK clause is likely pointless since the unsigned types seem to be used in Drupal for the expanded range of values, which you don't get with Postgres and a CHECK clause. You could bump unsigned int to bigint, but that would leave you without any recourse for bigints except to then switch to a numeric column. Messy.
#28
The patch in #20 works.
#29
#522786: Postgres numeric check needs to happen after precision check in schema generation is the very same patch as #20 is here (but with added tests), so marking postponed on that one getting into Drupal 7. I'm not supposed to commit changes before they land in D7, unless they do not apply to D7. As soon as that one is committed, please reopen this.