db_index_exists() missing, module updates cannot handle indexes properly

DROP INDEX (8.1)

Turns out db_drop_primary_key() and db_drop_unique_key() are also affected. I've added _db_index_exists() and used that in all three functions.

We need some reviewing of the patch since it's larger than trivial.

Patched code didn't work. Fixed it and rerolled.

Ran across the need for this today in #363262: Missing index on url_alias table.

1. Needs to be fixed in D7 first.
2. We already have db_column_exists() and db_table_exists(), so there's no reason to prefix this function with an underscore.
3. The function is missing PHPDoc.

webchick asked for my input here. I've no problem with an indexExists() method on the schema API, with a procedural wrapper. It should follow the same logical design as the rest of schema, of course, but I like having a rich set of primitives. :-)

Here is a rerolled patch with underscore removed and docs added.

Crell: Are you adding indexExists() or shall I assist?

Tagging

You can go ahead. :-) A D7 implementation, however, must put the actual logic back in the schema class, and then provide a DB-specific implementation in each DB's schema class. Then add a wrapper function that mirrors the other functions in database.inc.

Ok... i'll play with it over the weekend. Been waiting for another opportunity to get my hands dirty on your new DB stuff :)

Reposting the patch with the leading underscore removed on the calls to db_index_exists().

I fail to see how this is database specific. Isn't MySQL also returning some kind of error when you try to DROP a non existing index?

Plus, I'm -1 on this approach. Every inconsistency should throw a warning during the update process. It's the job of the administrator to choose to ignore them or not.

Damien,

I agree some feedback is required when the index you want to drop doesn't exist. It could mean duplicate indexes or that some column change operations will fail. In the case of duplicate indexes, it is a performance hit and doesn't break the system. In the case of failing column operations, the failed column operation (such as allowing dupes in a column with unique constraint) would fail showing an error.

To me, the issue is really about where to put the responsibility of error/warning reporting. Update code is the only part knowing the operations being performed and having the ability to report the condition appropriately.

Perhaps we don't bypass reporting of the failed index removal and make it a warning instead. This will allow an administrator to still see there was something fishy. An error on a following operation can also be associated with the warning without looking at code.

How does that sound?

I did a little dev on the D7 implementation of db_index_exists(). From my reading MySQL associates indexes with tables and PostgreSQL have them as independent entities. This will mean we have a function prototype of:

Most of it is implemented. Couldn't easily find information for sqlite. Can anyone offer some suggestions on this?

Here's a preliminary patch. I haven't tested the code yet.

Changing the title to match what's now going on in this issue. Still waiting on a review.

db_result() is deprecated. Use ->fetchField() instead.

Rerolled using ->fetchField().

Seems like this should add to the database tests. Just checking for an existing and non-existing index ought to work no?

The code looks good to me, but as catch suggests, it needs tests.

I'll get to it as soon as I can.

Bug in latest patch: $table needs to be escaped somehow when enclosed between {}. Otherwise table prefix substitution will fail.

Fix:

subscribing and promoting to PostgreSQL Sprint

I implemented this method to check for existence of indexes in CCK, which is available in CCK 2.5. Reference: #231453: Allow indexing columns.

But it has been reported (see #562260: content_db_index_exists has wrong syntax for postgres) that the following query doesn't work in PostgreSQL.

That it should look like this:

Is it possible that the contents of the indexname column varies depending on PostgreSQL versions? I've been unable to find information about how PostgreSQL stores data in this column, so I cannot really tell. :(

I hope the experience collected by the use of such a feature in CCK could potentially help provide a more reliable method for D7.

Does anyone knows more about the format of the indexname column in pg_indexes table throughout the different PostgreSQL versions supported by Drupal itself?

LOL... it was sooo much easy. The fact is that the Schema API for PostgreSQL is building the index names like that, so the statement to check if an index exists should be built using the same pattern for index names as the one used when creating the index. The following syntax is then correct:

And the one implemented in the patch in #17 is not, which is like this:

For references, see methods dropIndex() and _createIndexSql() in class DatabaseSchema_pgsql, implemented in file includes/database/pgsql/schema.inc

I hope that helps here. ;-)

I think this will be easily done with #302327: Support cross-schema/database prefixing like we claim to in. So lets wait for that. I'm sure this will go in after the freeze too.

Absolutely required. Also for the module update numbering scheme we teach in http://api.drupal.org/api/function/hook_update_N/7

If that helps, here's the code that's using CCK for a few months already:

use schema api instead of querying db directly

Chasing HEAD from #17, tidied the code to use the DB API properly (internal calls should always use $connection->query(), never db_query()), and adds unit tests.

Looks wrong. I'll correct that manually before committing. Interestingly, all tests pass.

Fixed, and committed to CVS HEAD. Thanks!

Weird, but thanks. :-)

Automatically closed -- issue fixed for 2 weeks with no activity.