If a module includes a $spec['length'] in its field schema then that length is simply added on to the end field type in _db_create_field_sql, which is a problem in postgres when that field type isn't varchar or character (there are other types where this syntax is allowed but drupal does not use them: see http://www.postgresql.org/docs/7.4/interactive/datatype.html).

A simple fix would be something like
if (!empty($spec['length'])) {
- $sql .= '('. $spec['length'] .')';
+ if(in_array($spec['pgsql_type'], array('varchar', 'character'))){
+ $sql .= '('. $spec['length'] .')';
+ }
}

but obviously it would be better to look at the length supplied and process it according to the type, eg an int of size 8 becomes bigint. However, the above fix at least allows modules like the computed field module to work with postgres, even if they length parameter will sometimes be ignored.

Thanks,
Steven H. Noble

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review

Would be great to have another Postgres fluent reviewer. Setting patch needs review since we have suggested code modifications.

Damien Tournoud’s picture

Version: 6.8 » 7.x-dev
Component: postgresql database » database system
Priority: Critical » Normal
Status: Needs review » Needs work

Ok, bumping to D7.

"length" is only meaningful for varchar, char and text, as documented in http://drupal.org/node/146939, but the documentation also says that this parameter is *ignored* for other datatypes.

This affect all drivers.

Damien Tournoud’s picture

Title: _db_create_field_sql can try to create types like int(8) which will fail in postgres » Schema API: 'length' should be ignored on non varchar and char fields
neilnz’s picture

subscribe

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
2.14 KB

Initial patch for review.

Dave Reid’s picture

Issue tags: +Schema API

Adding tag

Josh Waihi’s picture

Status: Needs review » Reviewed & tested by the community

@Dave Reid, I had a patch!, you got in before me, I'm happy, so lets get it commuted and backported to 6

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Let's add some code comments, please. Ideally, we'd update the tests too.

Josh Waihi’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.04 KB

This is already documented in includes/database/schema.inc, I merely updated it a bit.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Josh Waihi’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

@neilnz has a backport I believe.

neilnz’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.74 KB

Indeed I do. I've redone it slightly to mimic the D7 patch. Attached.

Josh Waihi’s picture

Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

awesome

Darren Oh’s picture

Patch works.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Committed to Drupal 6 too, thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Schema API

Automatically closed -- issue fixed for 2 weeks with no activity.