Add a 'number' column to the {acl} table [#936682]

ACL's main table has the following schema:

  $schema['acl'] = array(
    'description'     => 'The base Access Control Lists table.',
    'fields'          => array(
      'acl_id'        => array(
        'description' => 'Primary key: unique ACL ID.',
        'type'        => 'serial',
        'not null'    => TRUE,
      ),
      'module'        => array(
        'description' => 'The name of the module that created this ACL entry.',
        'type'        => 'varchar',
        'length'      => 255,
        'not null'    => TRUE,
      ),
      'name'          => array(
        'description' => 'A name (or other identifying information) for this ACL entry, given by the module that created it.',
        'type'        => 'varchar',
        'length'      => 255,
      ),
    ),
    'primary key'     => array('acl_id'),
  );

The name column is of type 'varchar' to keep all options open.

For Forum Access, the 'name' is the forum's tid, a number. This works fine with MySQL, as the types are converted automatically. However, PostgreSQL

requires a cast to convert the number to a string. In D6 this is implemented by adding the cast to the query string for 'pgsql'.

In DBTNG the join presumably looks like

    $acl_alias = $query->leftJoin('acl', 'acl', "%alias.name = $fa_alias.tid AND %alias.module = 'forum_access'");

DBTNG is smart enough to add casts to placeholders if needed, but I don't think this is done in join() $conditions, or is it?

Do I need to somehow find out the database type (how?) and conditionally add the cast to the query fragment?

Comments

Comment #1

Crell CreditAttribution: Crell commented 12 October 2010 at 01:56

Component:

database system

» postgresql database

Refiling as this is Postgres-specific. I seem to recall this question coming up at some point in the past, but I don't recall what the resolution was. It may have been "well you shouldn't be doing that kind of join, nitwit", but I could be making that up. :-)

Comment #2

salvis

he/his

CreditAttribution: salvis commented 12 October 2010 at 07:26

Well, "that kind of join, nitwit" goes back to merlinofchaos who designed the ACL schema, so I don't feel very guilty here. :-)

What should I do going forward?

Comment #3

Crell CreditAttribution: Crell commented 12 October 2010 at 15:00

See if you can find a similar closed issue somewhere. I think it had to do with either filter or blocks in Postgres and a query breaking in Postgres 8.3, when postgres got more picky about its joins.

Otherwise, let's drag DamZ in here.

Comment #4

salvis

he/his

CreditAttribution: salvis commented 17 October 2010 at 18:21

Here's what I found:

http://groups.drupal.org/node/9103#comment-28800
#220064-44: Ensure block delta is class safe

I'm beginning to think that the {forum_access} table should have a tid_string column that duplicates tid, but as VARCHAR(10), for the unique purpose of joining with acl.name. Would that make sense?

Comment #5

Crell CreditAttribution: Crell commented 18 October 2010 at 12:28

salvis, can you try the cast logic that agentrickard proposes early in the first link?

While I understand the desire for flexibility by using a varchar in ACL, that does break ANSI SQL in this case. What you're doing is not actually supposed to work AFAIK, so you'll need to either work around it or back out to just an int.

I suppose a denormalization column is a possibility, although it's a very hacky one. Maybe the PostgreSQL maintainers know more.

Comment #6

salvis

he/his

CreditAttribution: salvis commented 18 October 2010 at 21:23

can you try the cast logic that agentrickard proposes early in the first link?

I'm a bit challenged with this because I don't have a plethora of databases and database versions at my disposal for testing. Maybe we can discuss the alternatives first?

I suspect that the cast (or casts, as in #220064-9: Ensure block delta is class safe I suppose) may significantly slow down the join. Actually, the join is probably slow even for MySQL, because it has to do the same type conversion implicitly as if there were a cast. Providing the right types is likely to result in much better performance.

Changing ACLs schema from varchar to int is not an option. ACL is infrastructure, and just because FA puts a tid into that column doesn't mean that everyone else does the same thing. I don't think a denormalized column which is a straight copy of an existing column (except for the type) is such a bad thing. Not ideal, but definitely maintainable.

However, joining on int columns should be more efficient than joining on varchar columns, so we'd be going the wrong way here. I think I'll add a number column to the {acl} table, with both name and number allowing nulls. This will increase the size of the table by a nullable int column, but it will let each client module pick which one it wants to use, and it should provide better node access performance.

What do you think of this?

Comment #7

Damien Tournoud CreditAttribution: Damien Tournoud commented 18 October 2010 at 21:36

We don't support joining between types. Even if MySQL does allow this, you definitely should not do it, as it will skip most of the possible indexes in the join.

However, joining on int columns should be more efficient than joining on varchar columns, so we'd be going the wrong way here. I think I'll add a number column to the {acl} table, with both name and number allowing nulls. This will increase the size of the table by a nullable int column, but it will let each client module pick which one it wants to use, and it should provide better node access performance.

Feels like a decent way forward.