Support cross-schema/database prefixing like we claim to

According to both the discussion in #371: resolve ANSI SQL-92/99/2003 reserved words conflict in query statements and #103519: Install doesn't allow "." in db prefix for psql schema (that you vandalized), this should not be. A dot in $db_prefix is supported, and works for both MySQL (allows sharing of specific tables between instances of Drupal that are installed in different databases, with different credentials) and PostgreSQL (schema).

Well, that is another story if you are able to answer my idea point by point with enough support.

God, I already did. You simply *don't listen*.

The primary goal of prefixTables() is for table prefix (e.g. table_prefix_) replacement only, but not for database_name.table_prefix_ syntax (since tables are not located in same database):

Queries sent to Drupal should wrap all table names in curly brackets. This function searches for this syntax and adds Drupal's table prefix to all tables, allowing Drupal to coexist with other systems in the same database if necessary.

You have no argument there. You are simply stating your point of view, which is totally unsubstantiated.

The comment from default.settings.php never mention dot as valid syntax clearly, but only use alphanumeric and underscore as example valid database characters:

Be sure to use valid database characters only, usually alphanumeric and underscore.

That's a documentation issue that will need to be fixed, again this is not an argument.

Most online reference from drupal.org or even somewhere else never mention dot as valid syntax:

Here are a host of tutorials that explain how to setup multisites with shared tables:
http://drupal.org/node/43816
http://groups.drupal.org/multisite
http://www.practicalweb.co.uk/blog/08/08/07/drupal-multisite-shared-tabl...

Idem, that's a documentation issue.

By using database_name.table_prefix_ syntax we have a lot of limitation, e.g. both database must come with identical connection parameters, which result as similar effect of locate all different tables under same database instance:

This is because we only define ONE SET of connection parameter correctly. In this case the use of split DB is just equal effect with all tables located within single database instance. As MySQL come with nearly none of maximum number of tables limitation within single database instance, I can't find the usefulness of split DB handling.

That's really not true. In MySQL you can define *per table* and *per database* permissions. That means that you really can have the following setup:

• Site A has full access to database A with A credentials
• Site B has full access to database B with B credentials
• Site B has also access to some tables in database A with B credentials

Even if we define different set of connection parameters in settings.php, as no db_set_active() is ever called there is no actual effect:

The $db_url will only load during connection establish, where db_set_active() will take action for. As we do a tricky cross database switching with $db_prefix => database_name.table_prefix, Drupal is being cheated as all tables belongs to same database. It is function just because we hit the backdoor of both Drupal and MySQL, but not means this syntax is expected as valid :S

This don't mean anything know that I clarified that your previous argument is false.

When we are considering for escape all database constraint name as reserved word safe (e.g. http://drupal.org/node/371), [{tablename}] will not able to convert as `prefix_tablename` simply, since it may result as `dbname.prefix_tablename` (if dot exists in $db_prefix) which is invalid SQL syntax.

Well. I'm pretty sure you can easily change that. I don't see any limitation here, you are only being *lazy*. The world will not bend because you think it should. We will not drop a supported feature because you can't even program a simple replacement logic.

You vandalized one issue already, your points are heard there and in another and it's over. This won't happen sorry.

In my opinion blocking cross-db table sharing is unacceptable. While there may not be any theoretical limitation of having a thousand tables in a DB, there certainly are practical limitations. Trying to get novice developers to work with database dumps of all of that would be near impossible.

Hi,
interesting that my blog is referenced in this thread as an argument for preventing database prefixes

http://www.practicalweb.co.uk/blog/08/08/07/drupal-multisite-shared-tables

It's only since writing that post that I realised database prefixes are possible.

The project I am currently working on originally used two databases - one for "drupal" content and another for business data.

The other database was connected to using http://api.drupal.org/api/function/db_set_active/5

But the problem with this approach is that if an error occurs while connected to the non-drupal database - error handling attempts to log to the watchdog table in the wrong database

As a result we merged the two databases - this has had it's own problems.

We currently intend to use the database prefix method to use two databases - with one connection.

and I need to update my blog.

I'm strongly in favour of updating the documentation to refer to the database prefix

Changing this behaviour would break the sites of those who rely on it

see also
http://www.drupaler.co.uk/blog/multi-site-drupal-6x/55
http://drupal.org/node/291373 (Multi-site with single codebase, different content databases, shared user database, shared sign-on)

For Drupal 7, actually, the solution is for selected parts of core that *must* use the main database to bypass db_query() and access the default connection directly. Watchdog and the registry are both good candidates there.

Is it possible for a documentation bug to be fixed in current versions?

I'd like to +1 Damien's suggestion as well as Sean's proposal that the docs change is back-ported. Seems to me the documentation could stand to be clearer. Until Damien told me I could use the "dot" in settings.php, I didn't know - it's a great feature, but I've never seen any documentation about it. =)

It seems that "." is going to cause problems during updates since the way D5 and D6 check for table existence in postgresql doesn't take into account schemas.

Problem seems solved in D7 (does actually D7 support schemas?)

I still have to figure how to "backport" D7 solution to D5 and D6 in a clean way...
I've no idea about how/if D5-7 support schemas (including update path), so I'm not sure if backporting the solution of D7 is going to make any good to D5-6.

It looks there are still some problems to fix in INDEX creation in D7 if schema are going to be supported:

protected function _createIndexSql($table, $name, $fields) {
    $query = 'CREATE INDEX {' . $table . '}_' . $name . '_idx ON {' . $table . '} (';
    $query .= $this->_createKeySql($fields) . ')';
    return $query;
  }

#140860: Consistent table names and database handling of table names is currently debating implementing schema logic seperate from db_prefix logic. This makes sense since schema/db reference in db_prefix has been more accidental than intentional.

I do understand that using schema is a hack. But somehow it is a working manegable hack. (eg.PostgreSQL let you selectively restore schemas over existing DB and moving tables across schema preserve constraint and move sequences etc..).

This issue was about documentation. At the current state . in prefix is permitted but it may cause "solvable" issues.
From my point of view unless D7 fully support schema, a "." in prefix is still going to be permitted, but it is going to cause problems.
It would be nice if that was documented. It is going to be a reminder of the issues people may have to solve for D7, a warning to user updating etc...

I'm building up a reasonable experience with drupal, PostgreSQL and schema. If and when and where you're planning a patch let me know. Thanks.

As a side note: quoting object names may cause other "side effects". Names becomes case sensitive. SQL standard says objects with no quotes should be downfolded to uppercase. Unfortunately this is one of the rare cases where PostgreSQL doesn't follow the standard... and we may incur in other issues.
Furthermore... quoting is a pain ;)
PostgreSQL doesn't support writable VIEWS "out of the box", they may be ready for 8.4 (?) or they may be implemented with RULES... but I wouldn't go that route (at least now).

Schema support for PostgreSQL is currently broken. I've got a patch here to fix that. It also documents more in depth that schema support is available in Drupal as well as instruction on who to go about that.

This will need a decent review, also I think SQLite maybe able to use what I've got here, not to mention, MySQL might be able to get cross database support out of it too.

Marking as critical because PostgreSQL has support for schemas already in the driver layer, the fact that it doesn't work makes this a critical bug.

I've this apply to SQLite and MySQL also.

git diff's don't apply :(, here's a cvs one.

fixed up MySQL

The $info array at the top of getPrefixInfo() is badly indented.

getPrefixInfo() needs a complete docblock. I didn't immediately understand why $table has a default of "default". :-)

Method names should not begin with an underscore, ever. If they're non-public, make them protected.

+    $ret[] = update_sql('ALTER TABLE {' . $table . '} RENAME TO `' . $this->getPrefixInfo($new_name, 'table') . '`');

Er. $this->getPrefixInfo() returns an array, doesn't it? If it's returning a handle the prefixing of this thing" string, then getPrefixInfo() is the wrong name for it.

changes as requested

updated buildTableNameCondition to re-factor the newer code I've written.

ah, had to update tests. buildTableNameCondition no longer requires a prefixed table name

updating status

--no-prefix for git patches

Had MySQL working with a '.' based prefix but not without, works now.

1) I talked with Josh about this patch to see what was really going on.

2) Changing title to reflect what we're actually doing here. :-)

3) Setting RTBC.

The SQLite part is not quite there yet. SQLite uses:

CREATE INDEX databasename.indexname ON unprefixedtable (columns)

like PostgreSQL does.

Have altered and tested SQLite works now.

FYI: PostgreSQL doesn't follow the SQLite Syntax is uses:

CREATE INDEX indexname ON schema.table (columns)

re-roll

and for the test bot...

choice thanks!

+1, about to start on a multi-site multi-db development project, where this will be extremely useful.

what is stopping this getting in?

I found the documentation in this patch to be a little cryptic.

+++ INSTALL.pgsql.txt	29 Aug 2009 16:24:17 -0000
@@ -26,3 +26,19 @@
+3. CREATE A SCHEMA OR SCHEMAS (Optional Advanced)

Should be 'Optional advanced'.

+++ INSTALL.pgsql.txt	29 Aug 2009 16:24:17 -0000
@@ -26,3 +26,19 @@
+3. CREATE A SCHEMA OR SCHEMAS (Optional Advanced)

Should be 'Optional advanced'.

+++ includes/database/sqlite/schema.inc	29 Aug 2009 16:24:18 -0000
@@ -222,7 +232,12 @@
+    // The new name of a table doesn't need to be referenced by schema.
+    // In the situation where the db_prefix is something like schema.prefix,
+    // the query will fail. So we must figure this out here instead of wrapping
+    // the new column in curly braces.

Example comment that is hard to grok.

+++ includes/database/database.inc	29 Aug 2009 16:24:17 -0000
@@ -426,6 +426,25 @@
+   * Find the prefix for a table

Missing dot at the end of the sentence. We should also explain what the 'prefix for a table' is, and why it useful.

+++ includes/database/schema.inc	29 Aug 2009 16:24:18 -0000
@@ -165,6 +175,42 @@
+   * Create names for Indexes, Primary Keys and Contstraints.

We should use lower case letters here.

+++ sites/default/default.settings.php	29 Aug 2009 16:24:18 -0000
@@ -130,6 +130,19 @@
+ * You can also specify a schema as a part of the prefix if the schema 
+ * already exists, for example:

This is unclear. We should explain why you'd want to "specify a schema as part of the prexif if the schema already exists".

+++ INSTALL.pgsql.txt	29 Aug 2009 16:24:17 -0000
@@ -26,3 +26,19 @@
+  Drupal will run across different schemas within your database if you so wish.
+  By default, Drupal runs inside the 'public' schema but you can use $db_prefix
+  inside settings.php to define a schema for drupal to inside of or specify tables
+  that are shared inside of a separate schema. Drupal will not create schemas for 
+  you, infact the user that drupal runs as should not be allowed to. You'll need
+  execute the SQL below as a superuser (such as a postgres user) and replace
+  'drupaluser' with the username that drupal uses to connect to PostgreSQL with
+  and replace schema_name with a schema name you wish to use such as 'shared':

Always write 'Drupal' and not 'drupal' in documentation.

What about:-

$query = db_select("schema_name.table_name", 'st');

which failed because DatabaseConnection::escapeTable() would strip out the 'dot'. I don't see this fixed in the patch. This is very important when working through multiple schema in PostgreSQL. I'm not sure whether overriding escapeTable() in PostgreSQL driver is all we need to do.

@k4ml, identifying the schema in postgresql should done in settings.php and not in runtime. Thats the whole point of this patch - and not to introduce th abilty to identify the schema when using db_select

Here's a re-roll, along with some small changes to account for #570900: Destroy remnants of update_sql().

This one should be fixed. The two previous patches were missing an opening comment in includes/database/schema.inc...

Just comment cleanup, should be good to go.

looks great, thanks noahb :)

+++ includes/database/sqlite/schema.inc	14 Oct 2009 17:53:07 -0000
@@ -220,7 +230,13 @@
+    // the table with curly bracesi incase the db_prefix contains a reference

-i from braces.

+++ includes/database/sqlite/schema.inc	14 Oct 2009 17:53:07 -0000
@@ -220,7 +230,13 @@
+    $info = $this->getPrefixInfo($new_name);
+  	$this->connection->query('ALTER TABLE {' . $table . '} RENAME TO {' . $info['table'] . '}');

Indentation.

+++ includes/database/database.inc	14 Oct 2009 17:53:07 -0000
@@ -427,7 +427,26 @@
-   * Prepare a query string and return the prepared statement.
+   * Find the prefix for a table.
+   *
+   * This is not used in prefixTables due to performance reasons.
+   */

Uh. Could we expand these comments out? I don't understand why we have both tablePrefix() and prefixTables(). The docs should make it clear what does what and which one to use when.

+++ includes/database/database.inc	14 Oct 2009 17:53:07 -0000
@@ -427,7 +427,26 @@
+  /**
+   *    * Prepare a query string and return the prepared statement.

Formatting bug.

+++ includes/database/schema.inc	14 Oct 2009 17:53:07 -0000
@@ -165,6 +175,42 @@
+    if (($pos = strpos($info['prefix'], '.')) !== FALSE) {
+      $info['schema'] = substr($info['prefix'], 0, $pos);
+      $info['table'] = substr($info['prefix'], ++$pos) . $table;
+    }

Uh. Wow. I don't remember the last time I've seen ++$something.

Can we comment this function internally a bit to explain what's going on?

+++ includes/database/pgsql/schema.inc	14 Oct 2009 17:53:07 -0000
@@ -275,7 +275,12 @@
+    // The new name of a table doesn't need to be referenced by schema.
+    // In the situation where the db_prefix is something like schema.prefix,
+    // the query will fail. So we must figure this out here instead of wrapping
+    // the new column in curly braces.

If we're going to document this in only one place, let's do it in pgsql/schema.inc (though preferably in database/schema.inc.

+++ includes/database/pgsql/schema.inc	14 Oct 2009 17:53:07 -0000
@@ -275,7 +275,12 @@
+    $prefixInfo = $this->getPrefixInfo($new_name);
+  	$this->connection->query('ALTER TABLE {' . $table . '} RENAME TO {' . $prefixInfo['table'] . '}');

Indentation is off here.

+++ INSTALL.pgsql.txt	14 Oct 2009 17:53:07 -0000
@@ -26,3 +26,34 @@
+3. CREATE A SCHEMA OR SCHEMAS (Optional advanced)
...
+3. CREATE A SCHEMA OR SCHEMAS (Optional advanced)

Um. I'm seeing double. :)

And do we need a similar hunk for INSTALL.msyql.txt?

I'm on crack. Are you, too?

I've cleaned up the comments plus fixed up some code that was missed

using git diff --no-prefix origin/master. Should work.

Cycling the bot.

Bot, pay attention!

OK, reposting the same patch. Bot, wake up!

I forget, was this blocked on #633678: Make sequence API work on non-MySQL databases? Because that just went in. :-)

yes it was, my next focus

Would it be possible to support schema for functions too?

I think functions should have their own db_query call/method.
Advantages would be: auto uninstall as with tables, share function across schema even if they refer to tables in different schemas etc...
I'm doing it right now to install functions I use for fulltext search and well some support in core would come handy.

Is there a way to make drupal/db_prefix aware of shared "entities".
Now D7 should transparently support transaction and eg. modules that insert records at install time may make a mess if the module has some shared table.

Where would be the right place to talk about shared tables and search_path?
Once the module install phase is passed... stacking multi-sites with common tables is awesome with postgresql... and quite performant. Cloning sites is a matter of minutes.

BTW
I noticed CONCAT is still around but a) not tested, just || get tested and b) used just once in a place where 1) it could be changed to || 2) the query should be rewritten anyway.

Why you test existence
SELECT COUNT(*) FROM pg_proc WHERE proname =
just for concat and rand and not for other functions?

Finally:

select greatest(10::numeric, 15::numeric, 32::numeric); works perfectly under pg>=8.1
so there seems to be no need to define them with 3 arguments or with CASE (tested with null too and they are equivalent).

ivan, this is way off topic for this particular issue and since Drupal 7 is now in a code freeze, features can not be added however improvements to existing functionality is welcome but they need to have there own issues created and brought to the attention of the maintainers such as myself, DamZ and Crell.

Would it be possible to support schema for functions too?

Unfortunately, Drupal doesn't really know what a schema is, it just thinks of it as a prefix to the table. Because it doesn't know of any schemas other that 'public' we simply cannot create functions that reference tables from other schema. Is there a function inparticular your talking about? From memory, Drupal's postgres functions don't reference any tables.

In regards to concat, it must be used to align MySQL and PostgreSQL syntax - this is so contrib don't break postgresql support so easily.

Please feel free to email me if you have any other questions.

here is a re-roll. Have tested on PostgreSQL and it works.

lets try this.

+++ includes/database/database.inc
@@ -438,6 +438,26 @@ abstract class DatabaseConnection extends PDO {
+   * This is function is for when you want to know the prefix of a table. This
+   * is not used in prefixTables due to performance reasons.

At least a duplicate "is" here. Could be shortened a bit in general though.

+++ includes/database/mysql/schema.inc
@@ -25,7 +25,29 @@ class DatabaseSchema_mysql extends DatabaseSchema {
+    $info = array(
+      'prefix' => $this->connection->tablePrefix($table),
+    );

Why multi-line?

+++ includes/database/mysql/schema.inc
@@ -25,7 +25,29 @@ class DatabaseSchema_mysql extends DatabaseSchema {
+    if (($pos = strpos($info['prefix'], '.')) !== FALSE) {

We can remove the duplicate braces here.

+++ includes/database/mysql/schema.inc
@@ -25,7 +25,29 @@ class DatabaseSchema_mysql extends DatabaseSchema {
+  /**
+    * Build a condition to match a table name against a standard information_schema.

Wrong indentation.

+++ includes/database/pgsql/schema.inc
@@ -275,7 +275,12 @@ class DatabaseSchema_pgsql extends DatabaseSchema {
   function renameTable($table, $new_name) {
-    $this->connection->query('ALTER TABLE {' . $table . '} RENAME TO {' . $new_name . '}');
+    // The new name of a table doesn't need to be referenced by schema.
+    // In the situation where the db_prefix is something like schema.prefix,
+    // the query will fail. So we must figure this out here instead of wrapping
+    // the new column in curly braces.
+    $prefixInfo = $this->getPrefixInfo($new_name);
+    $this->connection->query('ALTER TABLE {' . $table . '} RENAME TO ' . $prefixInfo['table']);

This commment is a bit lengthy and could be shortened to the actual problem/action that is being performed.

+++ includes/database/pgsql/schema.inc
@@ -574,11 +576,11 @@ class DatabaseSchema_pgsql extends DatabaseSchema {
   public function getComment($table, $column = NULL) {
...
-    $this->connection->query('SELECT obj_description(oid, ?) FROM pg_class WHERE relname = ?', array('pg_class', $table))->fetchField();
+    return $this->connection->query('SELECT obj_description(oid, ?) FROM pg_class WHERE relname = ?', array('pg_class', $info['table']))->fetchField();    

Trailing white-space here.

+++ includes/database/schema.inc
@@ -154,7 +154,17 @@ abstract class DatabaseSchema implements QueryPlaceholderInterface {
+  
...
+   */ 

...and here (and possibly elsewhere)

+++ includes/database/sqlite/schema.inc
@@ -446,7 +462,7 @@ class DatabaseSchema_sqlite extends DatabaseSchema {
-    $this->connection->query('DROP INDEX ' . '{' . $table . '}_' . $name);
+  	$this->connection->query('DROP INDEX ' . $this->prefixNonTable($table, $name));

@@ -476,7 +492,7 @@ class DatabaseSchema_sqlite extends DatabaseSchema {
-    $this->connection->query('DROP INDEX ' . '{' . $table . '}_' . $name);
+  	$this->connection->query('DROP INDEX ' . $this->prefixNonTable($table, $name));

Wrong indentation, possibly tabs.

+++ sites/default/default.settings.php
@@ -131,6 +131,21 @@
+ * Example:
+ *
+ *  $db_prefix = array(
+ *    'default' => 'main.',
+ *     'users'      => 'shared.',
+ *     'sessions'  => 'shared.',
+ *     'role'      => 'shared.',
+ *     'authmap'   => 'shared.',
+ *  );
+ *

Missing @code + @endcode delimiters.

Powered by Dreditor.

I didn't apply the @code + @endcode delimiters becase no other code in default.settings.php has them (maybe that should be a separate issue)
Also this stays:

+++ includes/database/mysql/schema.inc
@@ -25,7 +25,29 @@ class DatabaseSchema_mysql extends DatabaseSchema {
+    if (($pos = strpos($info['prefix'], '.')) !== FALSE) {

because it is more explanatory than ($pos = strpos($info['prefix'], '.') !== FALSE).

Nothing of the following should hold back a commit. But of course, would be cool if those issues could be fixed either prior to commit or in a quick follow-up patch.

This issue is marked as critical, so this stuff really doesn't matter.

+++ INSTALL.pgsql.txt
@@ -26,3 +26,19 @@ Note that the database must be created with UTF-8 (Unicode) encoding.
+  that are shared inside of a separate schema. Drupal will not create schemas for ¶
...
+  Do this for as many schemas as you need. See default.settings.php for how to ¶

+++ includes/database/pgsql/schema.inc
@@ -413,8 +415,7 @@ class DatabaseSchema_pgsql extends DatabaseSchema {
+    $this->connection->query('ALTER TABLE {' . $table . '} ADD CONSTRAINT "' . $this->prefixNonTable($table, $name, 'key') . '" UNIQUE (' . implode(',', $fields) . ')'); ¶

@@ -574,11 +573,11 @@ class DatabaseSchema_pgsql extends DatabaseSchema {
+      return $this->connection->query('SELECT col_description(oid, attnum) FROM pg_class, pg_attribute WHERE attrelid = oid AND relname = ? AND attname = ?', array($info['table'], $column))->fetchField(); ¶

+++ includes/database/sqlite/schema.inc
@@ -220,7 +230,13 @@ class DatabaseSchema_sqlite extends DatabaseSchema {
+    // SQLite doesn't allow you to rename tables outside of the current ¶
+    // database. So the syntax '...RENAME TO database.table' would fail. ¶

Trailing white-space here.

+++ includes/database/schema.inc
@@ -165,6 +175,48 @@ abstract class DatabaseSchema implements QueryPlaceholderInterface {
+    // to the value before the period in the prefix. Everything after the dot with
...
+      // Grab everything after (++$pos increments $pos before it is used in substr).

+++ sites/default/default.settings.php
@@ -131,6 +131,21 @@
+ * or you want to access several databases from the same code base at the same time.

Exceeds 80 chars.

+++ includes/database/schema.inc
@@ -165,6 +175,48 @@ abstract class DatabaseSchema implements QueryPlaceholderInterface {
+    if (($pos = strpos($info['prefix'], '.')) !== FALSE) {

Still duplicated parenthesis here.

+++ includes/database/schema.inc
@@ -165,6 +175,48 @@ abstract class DatabaseSchema implements QueryPlaceholderInterface {
+      // Grab everything after (++$pos increments $pos before it is used in substr).
+      $info['table'] = substr($info['prefix'], ++$pos) . $table;

Unlike webchick, I don't think that ++$pos needs explanation. That's plain and totally valid PHP.

+++ includes/database/schema.inc
@@ -165,6 +175,48 @@ abstract class DatabaseSchema implements QueryPlaceholderInterface {
+   * Create names for indexes, primary keys and contstraints.

Typo in "contstraints".

144 critical left. Go review some!

Unlike webchick, I don't think that ++$pos needs explanation. That's plain and totally valid PHP.

I agree with sun, ++var statements are well-known in almost every programming language I know (that doesn't mean that there aren't languages that doesn't support it)!

if ++$pos is an issue; replace it with $pos++ above. Everyone knows what that does.

+++ includes/database/schema.inc
@@ -165,6 +175,48 @@ abstract class DatabaseSchema implements QueryPlaceholderInterface {
+      // Grab everything after
+      $pos++;
+      $info['table'] = substr($info['prefix'], $pos) . $table;

Actually looking at the code $pos is not used again so this is valid

+++ includes/database/schema.inc
@@ -165,6 +175,48 @@ abstract class DatabaseSchema implements QueryPlaceholderInterface {
+      // Grab everything after
+      $info['table'] = substr($info['prefix'], $pos+1) . $table;

Let's do a quick re-roll here. There was enough feedback on the code that it is worthy incorporating it. I agree that ++$pos is perfectly valid, but we should write $pos++ if we can.

@Dries: Why? Both appear to be identical functionality-wise in this instance. Technically ++$pos is infinitesimally faster. Neither one should require a comment as we can expect anyone writing PHP to understand it.

I don't really care either way, I just don't see why there's a preference.

Crell, sorry I wasn't suggesting we document ++$pos. I was suggesting we incorporate (some of) the feedback from reviews #70 and #72.

#69: 302327-reformatted.patch queued for re-testing.

re-roll to include comments from 70 & 72, including going with this:
+ $info['table'] = substr($info['prefix'], $pos+1) . $table;

Thanks noahb

Asked Crell for his final thumbs up (or thumbs down).

Let's get this long and storied issue committed. :-) I just read over the patch and didn't see anything to hold it up. (There were one or two stylistic things already in the code that this patch doesn't change; I am not holding this patch up for that.) Let's get this committed.

Josh, awesome work!

Committed to CVS HEAD. Thanks.