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 commentedRefiling 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
salvisWell, "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 commentedSee 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
salvisHere'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 commentedsalvis, 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
salvisI'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 bothname
andnumber
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 commentedWe 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.
Feels like a decent way forward.
Comment #8
Crell CreditAttribution: Crell commentedSounds like a plan. Refiling over to ACL as this is no longer a core issue.
Comment #9
salvisOk, thanks for your help!
Comment #10
salvisCommitted to the -dev version.
Comment #12
salvisBackport this to D6 to fix #968382: Large amount of mysql joins performed without indexes after forum access is installed.
Comment #13
salvisCommitted to the D6 -dev version.
We'll need this for #968382: Large amount of mysql joins performed without indexes after forum access is installed...