Add a primary key to the {taxonomy_index} table and prevent duplicates being inserted

Needs to be tested, assuming the bot works.

Also, shouldn't this "shortcut" table be tid/nid/vid instead of just tid/nid, just like term_node was ?

I won't say you're going crazy, but ... :-)

From http://drupalcode.org/viewvc/drupal/drupal/modules/taxonomy/taxonomy.ins...

$schema['term_node'] ... 'primary key' => array('tid', 'vid'),

term_node linking by vid instead of nid was one of the changes from D6 to D6.

Problem, of course, is that taxonomy_index does not contain a vid column currently, so there's no way to index on that missing field.

+1, subscribing.

re-rolled.

Using nid,tid. No vid, which is fine if I understand correctly. This is not like the old term_node table, which had to be revision-aware. This is a helper table to make queries more effiecent when displaying all nodes tagged with a given term.

The patch should be rolled from Drupal root.

Pretty sure this will fail on sites that have duplicates, we may need to delete those via a query before adding the index.

I do have duplicates in my table, so the update does indeed fail with:

The following updates returned messages
taxonomy module
Update #7011

    Failed: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '7-87' for key 'PRIMARY': ALTER TABLE {taxonomy_index} ADD PRIMARY KEY (`nid`, `tid`); Array ( ) in db_add_primary_key() (regel 2762 van ...\includes\database\database.inc).

I extended the patch to remove duplicates, which wasn't easy as this cannot be done with just 1 query. Update and delete queries may not refer to itself in joins/subqueries.

And now I now the reason why I had duplicates in my table: language code is not part of the primary key!

Upon translating a node I got the "Integrity constraint violation: 1062 Duplicate entry '24-129' for key 'PRIMARY': INSERT INTO {taxonomy_index}".

Some notes:
- I used the "old" method of translating, i.e making a new node and linking them via tnid. The fact that the fields of the new node have values in multiple languages looks like an error to me. I will file that separately.
- Because D7 also has translatable fields, this situation is to be expected and handled.
- This table is a denormalized copy of relationships between fields and terms. So it should contain the identifying parts of both:
* enity_type
* entity_id
* language
* delta
* deleted
* tid
- Restricting this to nodes (not other entities) this can be reduced to:
* nid
* language
* delta
* deleted
* tid
- Removing the "inner node" identifying parts, this can be further reduced to:
* nid
* language (strictly speaking this is inner node as well, but for this table to be usable it will be needed as most uses will want to find what nodes are related *given* the current language)
* tid

This means that duplicates remain possible and should be handled by the code inserting records.

I'm making this critical because I think that:
- Insert code is flawed.
- All read uses of this table are flawed in a multilingual situation.
- Repairing this might become a nightmare: the sooner the better.

Am I thinking right or not?
Should the title be changed or should this be a new issue?

The "critical" status is reserved for things that render Drupal completely unusable. This sounds pretty solidly "major" though.

subscribe

After applied #13, a db_insert('taxonomy_index') (in taxonomy module) throws PDOException.

Don't use the patch as it does not take away the cause, so, eventually, you will indeed get errors on insert.

This is blocked on #1050466: The taxonomy index should be maintained in a node hook, not a field hook, tagging as such. It's also a bug - once we have duplicates properly sorted out in the application code, the database should be enforcing it.

#1050466: The taxonomy index should be maintained in a node hook, not a field hook was committed; this should be unblocked now.

Does #1050466: The taxonomy index should be maintained in a node hook, not a field hook prevent duplicates being inserted (in multi lingual sites as well) or should we still check for that?

If so, this issue can be restricted to a hook_update that:

• Removes existing duplicates.
• Adds a primary index.

If not, the solution for this issue should be extended with preventing inserting duplicates by using a merge query instead.

Subsequently, if we want better support for multi lingual sites, we could (should) add a language column to the table and the primary index. questions:

• Should adding language to the table be a separate issue?
• Tag this (or the new issue) with D8MI?

If we can settle the scope of this issue, I will write a patch.

See also #1343822: Autocreated taxonomy terms duplicated if they are added to multiple fields. And yes, I think a merge query is a good idea.

I think anything for D8MI should be considered separately as there are much bigger implications for that.

OK, I will post a patch for D8 containing:

• Addition to taxonomy_schema() of a primary key index with order nid, tid. Order tid, nid seems to be worse as there are joins on specific nid's, whereas joins on tid are get all - taxonomy_select_nodes() - or count queries - taxonomy.tokens.inc - and thus don't need a unique index. If required the index on tid, sticky, created can be made unique by adding nid.
• Removal of the now superfluous index on nid.
• Removal of distinct in the query in taxonomy.tokens.inc (line 177) for token node-count for vocabularies). (Note: current query for token node-count for terms seems erroneous in the current case but will be corrected if the records are unique.
• Replace db_insert by db_merge in taxonomy_build_node_index().
• Replace the use of db_insert by db_merge in the examples in field.api.php.
• The current testTaxonomyIndex does covers the new situation.

The patch for D8 does not need to contain an update, assuming that the backport to D7 gets in and released in an official version before D8 is released... How to assure this?

The patch for D7 will contain:

• Same changes as for D8.
• An update hook to remove duplicates.
• An update hook to make the changes to the schema.

I will create a new issue to find out if we want to add language to this table and its indexes.

I don't understand this as a bug, if it is, can someone update the issue summary explaining it?

Talked to @tim.plunkett about this issue in IRC. This is a gnarly bug, see #23 and #20 and #14. And yeah, an updated summary would be good.

Also unassigning.

FWIW, we also tackled that issue in entityreference: #1586428: Integrate entyreference from nodes to terms with core taxonomy_index, where it now seems to be solved, pending further tests.

...friendly bump ;)

I've rolled a patch per @fietserwin's plan in #24 - I did add an update hook, leaving removal of dupes as a @todo for now. I didn't see a relevant example in field.api.php to change.

OT: No API to find if a table already has a PK? It would be nice to skip the dupe removal step if the PK is already in place...

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -386,3 +387,21 @@ function taxonomy_update_8007() {
+  // @todo: Remove duplicates on nid/tid, keeping the row with the most recent
+  // created date.
+  try {
+    db_add_primary_key('taxonomy_index', array('nid', 'tid'));
+  }
+  catch (Exception $e) {
+    // Simply ignore if the primary key already exists.
+  }

Without this @todo, the creation of the primary key will fail if there are duplicates (and there are Drupal installations out there with duplicates). Worse, it will fail silently, and the user can continue as if the update took place (but it never did). Is it possible to just catch the case where a primary key already exists?

Thus this dupe removal MUST be solved before it can be committed.

But my patch from #13 contains the code how to do that in D7. So it should be easy to port to D8 (I hope). One note though on that patch: I'm not sure about if and how to divide that in batch runs as I guess it can run for ages on large databases.

BTW: If a primary key already exists how can we be sure that it is the same (same fields, same order)? So we should delete it first and then create it the way we want it to be. We should then only catch the removal of the primary key (if that throws an exception) and not the creation of it. (This also answers the last question of #30: we cannot skip the dupe removal step as we don't know what the existing PK looked like.)

The table could be truncated before adding the primary key - it'll get rebuilt on cron anyway.

#32: I didn't know that. That would be the easiest way to solve. i had thought about doing that in the hook_update, but this is far easier.

Thus hook_update should: truncate, drop nid index, remove primary key (fail silently), add primary key.

@catch - I'm not seeing how this would be rebuilt on cron, as far as I can see taxonomy_index is only written by taxonomy_build_node_index(), which I only see invoked in hook_node_insert() and hook_node_delete(). Maybe it's forum_index that gets rebuilt automatically?

@fietserwin: Yes, I knew it wasn't committable as-is, just wanted to feed what I had to the testbot.

So, looks like we do need to rebuild the table ourselves, and we will have to batch it.

Interesting, it's not. I must've been thinking of tracker.

#35: So, looks like we do need to rebuild the table ourselves

There is (D7) dupe removal code in #13

Yes, I'm looking on batchifying your #13 dedupe code, although if you want to do it yourself by all means go ahead, if I don't find the time to do that today I won't have another chance for a couple of weeks.

Added Issue Summary

Added batchifying for removing duplicate nid/tid per #13. Change sticky in case there was nid/tid with and without sticky.
Please test and review.

Forgot to change the limit to 500 as i testede it with 1. New patch. Disregard the #38 patch.

Somehow the patch got corrupted. Here we go again.

In general: if migrate in core is going to make it, this whole hook_update() exercise may be of no value.

1. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -396,3 +397,68 @@ function taxonomy_update_8008() {
   +  if (db_index_exists('taxonomy_index', 'nid')) {
   +      db_drop_index('taxonomy_index', 'nid');
   +    }
   

   The index is still in hook_schema(), adapt the schema as well.

2. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -396,3 +397,68 @@ function taxonomy_update_8008() {
   +  // Remove duplicates on nid/tid, keeping the row with the most recent
   

   (End comments with a period.)

   I know this code comes from a patch I posted, but I then misunderstood the field created: it is the create date of the node, not of this record. So it is always the same (unless someone played with that in the database).

   So we could just retrieve duplicate nid/tids form taxonomy_index and join these with the node table to get the created and sticky values. doing so this way, we also "update" the table to reflect current value (for sticky).

3. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -396,3 +397,68 @@ function taxonomy_update_8008() {
   +  $conditions = & $delete->conditions(); // Returns by reference,
   +  $conditions['#conjunction'] = 'OR'; // so we can change it into an OR.
   

   Move the comments to 1 comment line above these 2 lines of code.

Clarified issue summary.

Ok, here is another patch after review.

Change status. Where did all the comment stuff where we could change the status and upload go after the upgrade???

There was a patch conflict. This is now resolved. See new patch.

If you also want to update the status, or other fields, click on "Update this issue" instead of fiilling in the comment right on this page.

As per https://groups.drupal.org/node/402803, the hook_update_n should be removed from the patch. This makes this issue an easy one. However, I am not sure whether a separate migrate issue will be needed, so I will file one as per the instructions

Migrate issue: #2186929: Migrate {taxonomy_index} table: table now has a unique primary key

New patch, without hook_update_n function to remove duplicates. This won't be needed with migrate in core.

Patch will fail with #2232037: taxonomy_index should have a primary key committed. Let's also only post the other part:

Adapted the patch to the fact that the node id is now read using a method.

Tested patch and db_merge here works nicely :)

If db_merge supports multiple merges at once that would be nice but I don't think it does currently so this syntax looks good. +1 for RTBC.

D7:
Using ALTER IGNORE TABLE seems like the best way to add the index and drop duplicated rows in an update function.

Thanks for reviewing.

- db_merge like update does indeed not support multiple sets of values to be merged (with an update you can only update a set of (fields of) records to the same new values/expressions).
- D7 solution: looks good, but is this supported by all db engines supported by Drupal? I couldn't find an y info about this for Postgres. But this is for the backport and in fact we already have a kind of working hook_update_n().

Setting to RTBC based on #56, and my 1st comment above responding to the concern mentioned in #56, and the fact that HEAD is kind of broken by committing issue #2232037: taxonomy_index should have a primary key.

It's a shame we lose the multi-insert, but don't see a way around that. Committed/pushed to 8.x. Moving to 7.x for backport.

Thanks for this quick reaction. Good to have this in D8:
- updated issue summary to make it more D7 specific.
- unblocked the migrate issue.

Has anyone made any progress on the D7 backport?

Here's a quick patch, one without update (do not test) for myself and one with an attempt at the update

oops

Patch looks good

1. +++ b/modules/taxonomy/taxonomy.install
   @@ -913,5 +913,25 @@ function taxonomy_update_7011() {
   +  db_add_primary_key('taxonomy_index', array('nid', 'tid'));
   +}
   

   I guess it is better to first drop a primary key if it exists. People may have created one themselves. If no primary key exists it is a harmless no-op.

I tested, in MySql, the:
- add field statement: adding the key at the same moment as the serial field is crucial.
- delete statement, because I got errors when referencing the same table in a sub-query (#13), but apparently joining to the same table is OK.

So, after fixing the minor point mentioned above, this is OK.

can anyone help me reproduce this error....??

The D7 -> D7 upgrade path will also need test coverage.

I received this error message after applying the patch from comment #64 to my taxonomy module (v.7.31) and then trying to run the update script:

Update #7012

Failed: PDOException: SQLSTATE[42000]: [Microsoft][SQL Server Native Client 11.0][SQL Server]Incorrect syntax near the keyword 'WHERE'.: DELETE t1 FROM {taxonomy_index} as t1 join {taxonomy_index} as t2 WHERE t1.nid = t2.nid AND t1.tid = t2.tid and t1.tempid > t2.tempid; Array ( ) in taxonomy_update_7012() (line 950 of modules\taxonomy\taxonomy.install).

When I look inside if the taxonomy_index table, I don't see a tempid column, and these are the columns that are still there: nid, tid, sticky, created, and __pk.

Am I missing a step?

When millions of records in taxonomy_index table then this apporach takes couple of hours or may be more than a day to finish.

is creating a new table of the new schema and guaranting to be distinct using:
INSERT INTO taxonomy_index_new SELECT DISTINCT t1.* FROM taxonomy_index t1 JOIN taxonomy_index t2 ON t1.nid=t2.nid AND t1.tid=t2.tid AND (t1.created > t2.created OR (t1.created = t2.created AND t1.sticky > t2.sticky)

acceptable? - use GROUP BY t1.nid, t1.tid, t1.sticky, t1.created if DISTINCT not portable enough but it seems to be.

and then (using db functions)

RENAME TABLE taxonomy_index TO taxonomy_index_old
RENAME TABLE taxonomy_index_new TO taxonomy_index

then catch up on bits using old table if needed before dropping it.

INSERT INTO taxonomy_index SELECT DISTINCT t1.* FROM taxonomy_index_old t1 LEFT JOIN taxonomy_index t2 ON t1.nid=t2.nid AND t1.tid=t2.tid AND t2.nid IS NULL

Any updates?

Bump for updates.

Rerolled.

Going back to 'needs work' because it needs tests and other improvements.

It might be a little scary, but would it be easier to just delete all data in the table, update the schema and recreate the data, rather than locking up the database by adding a new index to a (possibly) giant table?

I've found that when modifying the primary key it's best to lock the table; makes it run faster. Also checking to see if the update needs to be done is usually a good idea.

    $results = db_query("SHOW KEYS FROM {taxonomy_index} WHERE Key_name = 'PRIMARY'")->fetchAllAssoc('Column_name');
    if (count($results) < 2) {
      db_query('LOCK TABLES {taxonomy_index} WRITE');
      // Preform update.

      db_query('UNLOCK TABLES');
    }

I've a database with > 130k nodes and ~80k terms... the taxonomy_index contains ~12000k records. Any record is duplicated even more than ten times each.

Assuming it would be safe to delete the redundant data, how can we avoid to get new duplication?

Will updating the table schema and applying drupal-n610076-75.patch be safe enough?

Thanks for your feedback!

@mikeytown2 Would you mind adding #78 to the patch?

ALTER TABLE taxonomy_index ADD PRIMARY KEY(nid,tid);

trying to generate sql structs in Go to access a D7 installation and taxonomy_index table was giving me an error.
After reading through this issue the above mysql query added a composite primary key on nid and tid fields.

I'm now having other issues but those aren't related to taxonomy

Added a new patch that checks both if the index exists and first removes any existing primary key before adding the new one, so any manually added primary keys will be dropped.

In our website we have 263388 taxonomy terms and hook_update taxonomy_update_7012 takes more than 10h for execution.
I'm providing a new version that reduce the execution time to less than 2 minutes.

Patch refactoring

D7 patch works great.

D7 patch works only on MySQL. The SQL query syntax is causing tests failures in SQLite and PostgreSQL.