On pgsql, system.install does CREATE DOMAIN {,small,big}int_unsigned CHECK (VALUE >= 0) during installation. This works fine, but during the Schema API now supports more kinds of unsigned types (float and numeric). So we either need to create unsigned domains for those types as well or put CHECK constraints on all unsigned columns.

The issue http://drupal.org/node/60871 discussed domains vs. check constraints and although domains ended up being used there did not seem to be a strong argument for one over the other.

Now, there is an argument: All simpletests throw exceptions on pgsql because the test re-runs the installation against a prefixed set of tables. The domains are not prefixed, however, so we get "already exists" errors when the install tries to re-create them.

Now that we have Schema API generating our CREATE TABLE SQL for us, I think the simplest solution is to use CHECK constraints on all unsigned columns.

There is an update gotcha here. We should delete the domains once we are no longer using them, but that means the system update will have to convert all the tables in the database to use a CHECK constraint even if the table does not exist in the schema (some module may not be enabled yet). Schema.module's inspection code can make this possible, but then system.install depends on a contrib module. This will take some thought.

Comments

bjaspan’s picture

bjaspan commented

I filed http://drupal.org/node/257009 for the related (but not duplicate) simpletest bug.

I guess I'll add that the reason they are not duplicates is that this bug is about an unnecessary lack of symmetry in the way int, float, and numeric unsigned columns are handled. http://drupal.org/node/256001 fixed the issue of float and numeric unsigned not being supported, and has been committed, but now we have two ways of doing the same thing.

bjaspan’s picture

bjaspan commented

I fixed the problem with simpletests reported in http://drupal.org/node/257009 with a simpler workaround. Now, the only motivation to fix this issue is that unsigned constraints are enforced via two different mechanisms in the pgsql driver: one for ints, another for serial, float, and numeric. It works fine this way, though, and maybe we should just leave well enough alone.

I'll leave this open for now but I anticipate a 'wontfix' coming in the future.

linuxpoet’s picture

linuxpoet commented

Actually there is an argument about check constraints against domains. Domains are slower.

Crell’s picture

Crell commented
Component: database system » postgresql database

PostgreSQL maintainers, your thoughts? What shall we do here?

josh waihi’s picture

josh waihi commented

I'm against domains as PDO doesn't support custom data types very well. We had this issue earlier and took out a how bunch of domains I thought. I can't see and domains in D7. grep -irn 'create domain' *

josh waihi’s picture

josh waihi commented

We've already addressed this. See this snippet as an example from createFieldSql:

    if (!empty($spec['unsigned'])) {
      $sql .= " CHECK ($name >= 0)";
    }
Crell’s picture

Crell commented

So is this fixed? Are there update hooks needed to clear out old domains from upgrading installs?

josh waihi’s picture

josh waihi commented

No, thats the only thing left todo I think.

josh waihi’s picture

josh waihi commented
Issue tags: +PostgreSQL Surge

Marking this as apart of the surge. Need to define the upgrade path.

josh waihi’s picture

josh waihi commented
Priority: Normal » Critical

An upgrade path needed makes this issue critical.

josh waihi’s picture

josh waihi commented
Title: On pgsql, replace domains with constraints » PostgreSQL upgrade path: Domains datatypes to constraints

naming more appropriately

damien tournoud’s picture

damien tournoud commented
Priority: Critical » Normal

Given the level of breakage of Drupal 6 on PostgreSQL, this cannot be possibly critical. I would be already very happy if Drupal 7 ships with 100% pass rate on PostgreSQL, even without any upgrade path.

josh waihi’s picture

josh waihi commented
Status: Active » Closed (fixed) 
/**
 * Remove custom datatype *_unsigned in PostgreSQL.
 */
function system_update_7016() {
  // Only run these queries if the driver used is pgsql.
  if (db_driver() == 'pgsql') {
    $result = db_query("SELECT c.relname AS table, a.attname AS field,
                        pg_catalog.format_type(a.atttypid, a.atttypmod) AS type
                        FROM pg_catalog.pg_attribute a
                        LEFT JOIN pg_class c ON (c.oid =  a.attrelid)
                        WHERE pg_catalog.pg_table_is_visible(c.oid) AND c.relkind = 'r'
                        AND pg_catalog.format_type(a.atttypid, a.atttypmod) LIKE '%unsigned%'");
    foreach ($result as $row) {
      switch ($row->type) {
        case 'smallint_unsigned':
          $datatype = 'int';
          break;
        case 'int_unsigned':
        case 'bigint_unsigned':
        default:
          $datatype = 'bigint';
          break;
      }
      db_query('ALTER TABLE ' . $row->table . ' ALTER COLUMN "' . $row->field . '" TYPE ' . $datatype);
      db_query('ALTER TABLE ' . $row->table . ' ADD CHECK ("' . $row->field . '" >= 0)');
    }
    db_query('DROP DOMAIN IF EXISTS smallint_unsigned');
    db_query('DROP DOMAIN IF EXISTS int_unsigned');
    db_query('DROP DOMAIN IF EXISTS bigint_unsigned');
  }
}

That cleaned out all old domains and updated existing domains. That makes this issue fixed.