Table names might be too long for PostgreSQL

The code actually already checks for the table name being at most 63 characters long (which is the limit for PostgreSQL 8.3) – however, I forgot to take the database prefix into account.
The attached patch should fix this, please try it out.

I checked it and it only works when a suffix is needed.
Attached a fixed patch.

Ah, you're right, thanks for fixing!
Committed.

affected module version: included Search API module in commerce kickstart V2 (7.x-2.0-alpha3 / 7.x-2.x-dev) distribution.

mmmhh...

I guess the table names are still too long for PostgreSQL 8.3 databases even if no prefix is used.

The installation of commerce kickstart V2 (7.x-2.0-alpha3 / 7.x-2.x-dev) fails with the following error:

SQLSTATE[42P07]: Duplicate table: 7 ERROR: relation "search_api_db_kickstart_product_display_field_product_variation" already exists
I tried this with the development version commerce_kickstart-7.x-2.x-dev-core.tar.gz (28.07.2012).
By the way the table name has 63 characters.

This issue seems to be very similar to the issue that I have posted before: http://drupal.org/node/1545936

When I reported the relating bug http://drupal.org/node/1545936 my the table name had 62 characters including a table prefix of 19 characters and I had the same error.

According to the PostgeSQL specifications table names with a maximum of 63 charcters are supported but the PostgreSQL specifications seems to be wrong.

My solution for the "old" commerce kickstart V1 version (7.x-1.6) was to shorten the table prefix to 2 characters that length of table name had only 45 characters.

=> The maximum supported table name length must be somewhere between 45 and 60 characters.

Would it be possible to shorten the table name length to 45 characters or to a number of characters that is supported by PostgreSQL?

Here is my configuration:
commerce_kickstart-7.x-2.0-alpha3-core.tar.gz and commerce_kickstart-7.x-2.x-dev-core.tar.gz (28.07.2012 + 03.08.2012) tested
PostgreSQL 8.3
PHP 5.3.2
Lighttpd 1.4.20

greetings
Metulski

WORKAROUND (but not really comfortable)

I compiled the latest PostgeSQL 9.1.5 version with the "#define NAMEDATALEN 128" (default value 64 of file pg_config_manual.h ) and there was no error at installation of the "search api module" included in the kickstart V2 beta2 distribution anymore.

But I guess (re)compiling PostgreSQL with changed default settings as a workaround will not be an acceptable workaround for the majority of search api / commerce kickstart users.

By the way I found an hint that might explain why the theoretically supported 63 characters for table names cannot be used completely here:
http://www.postgresql.org/docs/9.1/static/runtime-config-preset.html
"The default value of NAMEDATALEN is 64; therefore the default max_identifier_length is 63 bytes, which can be less than 63 characters when using multibyte encodings."
The UTF8 character set / encoding is a "multibyte encoding" that is required for drupal.
http://www.postgresql.org/docs/9.1/static/multibyte.html

The patch for this issue does nothing to address the problem of similar name collisions. If you just trim the name, you will have a collision if you have anything with a similar name.

"search_api_db_product_display_" + "field_product_commerce_price_amount_float_asc"
"search_api_db_product_display_" + "field_product_commerce_price_amount_float_desc"

Drupal Commerce Kickstart RC1 is running into this issue:
#1801380: Install does not work on PostgreSQL due to 63 character table and column name limit

Views is attempting to solve this same issue by creating an MD5 hash of the name:
#571548: Identifiers longer than 63 characters are truncated, causing Views to break on Postgres

Based on #7 I think we should not use drupal_substr since this will not cut the string down to a certain length in bytes, but to a length in characters. We should instead use mb_strcut(). This will cut the string down to a certain length in bytes so that it will always fit in the 63 bytes that Postgres allows.

drupal_strlen() has the same problem. strlen(), will give the actual length of a string in bytes, so in this case we do actually not want to use the wrapper functions.

May I note that this bug renders Commerce Kickstart completely unusable when running on PostgreSQL. Recompiling Postgres is not an option for most users.

Attached is a patch that implements a check on length in bytes.

It is odd, but using 63 as maxbytes still doesn't work. 62 bytes however does cause Postgres to be happy.

By the way. This problem shouldn't occur only in the Search API. Isn't it a better idea to add some kind of "findFreeTable" function in the database drivers so that every driver can implement its own restrictions and solutions?

In addition, I suggest to short even more the max allowed length for table name to 51 because of simpletest. Simpletest copies all tables to the same database but adding a preffix (usually simpletestNN) to perform its tests.

This means that 12 extra characters are needed to copy database with Simpletest prefix.

this may also happen with mysql, see #1704768-14: /search/job/ issues «SQLSTATE[42S02]: Base table or view not found: 1146 Table 'recruiter.recruit_search_api_db_job_search_sea».

Thanks a lot for the patch, xatoo!
Can anyone else please confirm this works for them on PostgreSQL?

In addition, I suggest to short even more the max allowed length for table name to 51 because of simpletest. Simpletest copies all tables to the same database but adding a preffix (usually simpletestNN) to perform its tests.

Are you positive this doesn't already work? I think the existing code's regarding of prefixes should already account for this.

This fix only causes another error. You'll end up with table name collisions. Drupal core needs to find a real solution to this problem or module maintainers need to keep their table names as short as possible.

#13 Keep in mind that this is not a standard Drupal prefix problem, is a problem when simpletest is used, because it adds a prefix that wasn't there before. When this module creates its tables doesn't know anything about Simpletest prefix, because is not even used. After in time Simpletest may be used and all tables will be copied with a new Simpletest prefix.

Could you please stop inventing imaginary problems and just test the patch (if the problem occurs for you)?
Thanks.

Sure. I've tested but it fails.

Let me explain better.

It's my fault I didn't expain it fails when a complete DB clone is made, for example using Simpletest Clone or behat_testing. Those modules test using a complete DB copy, not only a clean Drupal installed site as Simpletest does.

That means that any table name greater than 51 characters can't be copied because of the max table name limit. In my case the index name. In my installation (dev with patch applied) it fails when search_api_db_search_api_db_test_index2_very_very_very_long_fi table is copied:

SQLSTATE[42000]: Syntax error or access violation: 1103 Incorrect table name 'simpletest101search_api_db_search_api_db_test_index2_very_very_very_long_fi'  

As you can see, 'simpletest101search_api_db_search_api_db_test_index2_very_very_very_long_fi' is 76 characters long.

I'm using behat_testing.

As you can imagine this is not only a search api db issue, but of any module that creates too long table names. I thought that as table name length is discussued in this issue it was a good idea to point this out.

As you can imagine this is not only a search api db issue, but of any module that creates too long table names. I thought that as table name length is discussued in this issue it was a good idea to point this out.

Oh, OK, then I'm sorry.
Still, this is clearly a problem of these other modules, not our module here. Copying tables and then adding things to the table name is just a stupid idea if there is a maximum limit the name length. I admit, it is a tricky problem to solve, but it still can't be the job of all other Drupal modules to mind the additional restrictions imposed by these modules.

Could someone (who had this problem – Metulski?) please test the patch in #10 and confirm it works?
Thanks!

I agree that patch #10 is not the right answer -- simply cutting off the end of the table name (as I understand it) is a band-aid solution that will lead to name collisions in the future. I'd rather see a more elegant (and permanent) solution put in place.

Has any of you "this will lead to collissions" people actually read the patch? Let alone tried it?

+++ b/service.inc
@@ -122,22 +122,24 @@ class SearchApiDbService extends SearchApiAbstractService {
     while (db_table_exists($table)) {
       $suffix = '_' . ++$i;
-      $table = drupal_substr($base, 0, $maxlen - drupal_strlen($suffix)) . $suffix;
+      $table = mb_strcut($base, 0, $maxbytes - strlen($suffix)) . $suffix;
     }

This will clearly change the table name as long as a table with that name exists. No collission possible. (If there is an error in that code, please enlighten me.)

So, again the request: can anyone who has that problem (or access to a Postgres server) please try the patch?

I just tried #10 on Commerce Kickstart and this patch does fix the problem. Commerce Kickstart installed without and errors and I now have tables like this:

search_api_db_product_display_field_product_commerce_price_amo
search_api_db_product_display_field_product_commerce_price_a_1
search_api_db_product_display_field_product_commerce_price_a_2

I have no idea if this will fix the simple test issue. They need to do something similar to table names they create.

Thanks xatoo and drunken monkey. Sorry for giving you a hard time about this issue.

#10: search_api_db-table_name_max_length-1214846-10.patch queued for re-testing.

Great, committed. (No idea why the test bot is going crazy, but local tests finish fine.)
Thanks again for fixing this, xatoo!

Owwkeej. I sure hope it really works now since I was not that convinced myself...