Errors: The following table(s) do not have a primary key: forum_index

I think we could make the 'forum_topics' index the primary key

Please review.

Thanks, left a comment on the MR

Please review.

Now we need 2 tests:
1. A kernel test, that after we install the forum module and have created its tables, the table "forum_index" has a primary key and that it is based on the fields "nid" and "tid"
2. A update test, that before we run the update process the table does not have a primary key and the we run the update it does.

Is it possible to use forum module under Drupal 10.1.x given it throws errors about this?

My test update page from 10.0.x to 10.1.x now shows this error on status page about this problem and I am not sure what exactly this means for the website operation?

If this breaks websites using forum, shouldn't this be higher than normal priority?

Hmm, I am a bit confused. I use D10 and the Forum is still in the core. I also don't recall the forum module being marked as deprecated compared to other modules moved to the contrib.

We made a copy into contrib in preparation for removal from core but it didn't get done in time.

We still plan to do it during the D11 cycle, its just proven tricky

I am sorry for asking that much, but I think I did not get the answer yet, it may also be relevant to others (all other people updating to Drupal 10.1.x and using forum module):

Is it possible to use forum module under Drupal 10.1.x given it throws errors about this?

Should I keep D 10.0.x, not use READ-COMMITED, ignore the error, manually add primary key to the table (which columns?) or what action is recommended to the users of D10+forum until this gets merged?

@KlemenDEV ideally we all work to get this into the next bugfix release of 10.1 (July)

Bumping priority now that 10.1 is out

#10 lists remaining items, I'll copy that to the issue summary

Ok, thank you for the clarification and the work on this issue :) I will wait for the next 10.1 release

Rebased the MR off 11.x and added those tests

Could the namespaces be off? Not sure "Unable to generate test groups" mean.

Added the missing @group

Thanks!

Don't mind marking this

Only question should MR be updated for 11.x and update hook 10201 instead for 10.2

I think we might want to consider backporting this, so I went with a 10.1 series update hook number.
But obviously can easily update if needs be.

I don't have a strong opinion whether this should target 10.x or 11 since the SQL transaction isolation level stuff is a bit too low level stuff for me. But with this background and disclaimer:

1. Do we have concerns or policies why we would NOT include this in the 10.x series?

2. If yes, should we at least then update the issue summary so that we explain the impact what this error in the status report means?

• I mean, it's not classified as a warning in the status report, it's highlighted as an error.
• So I would assume that many, many site builders will find their way to this issue and if we're only going to ship this in D11, it would be extremely beneficial to tell in the issue summary what the impact of this is.
• And in that case we would probably also want to provide a link to other docs that will give guidance for the site builders how they can apply the patch if they don't want to wait until D11. (I don't need those instructions myself, but I can imagine many, many site builders feeling very uncomfortable when the status report is displaying errors).

Cheers,
Markus

We normally avoid backporting update hooks to patch releases because even simple update hooks can cause problems on updating.

Adding an index is however one of the more simple things we can add, and the worst that happens if it goes wrong is the error message persists.

If we didn't backport, another option would be removing the hook_requiremenths() message and/or filtering out forum_index from it, but in this case I think backport is probably the right option.

One comment on the MR about the update message in the case there's no table.

NW for the change to the message

Made that change, thanks

All code changes look good to me.
The PK has been added.
An update hook has been added.
For both testing has been added.
For me it is RTBC.

Will this now target 10.1 or 11.x? It is a bit concerning if target is 11 as this means all users using forum module and MYSQL will have to live with error on status page for the whole D10 lifetime.

@KlemenDEV: We are first doing 10.1 and 11.x. After that has landed we will backport it to 10.0 and 9.5.

Closed #2979425: Table forum_index needs a primary key to work on Percona as a duplicate, adding credit.

Committed/pushed to 11.x and cherry-picked to 10.1.x, thanks!

I don't think we should backport the update to 10.0.x/9.5.x, but maybe we should exclude forum_index from the error message, so leaving open against 10.0.x to discuss.

I could be wrong, but I don't see the message about isolation level/PKs in 10.0.x so perhaps this is fixed? I.e the issue doesn't exist in 10.0

Closed #2931620: Forum module can generate integrity constraint violations (duplicate primary key) when saving changes to existing non-default node revisions (can be triggered by d6_term_node_revision migrations) as a duplicate, moving credit.

Looks like these commits broke HEAD of 10.1.x and 11.x on both pgsql and sqlite with a friendly:

1) Drupal\Tests\forum\Functional\ForumIndexUpdateTest::testUpdatePath
The link Continue was not found on the page.
Failed asserting that an array has the key 0.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:406
/var/www/html/core/tests/Drupal/Tests/UpdatePathTestTrait.php:48
/var/www/html/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:247
/var/www/html/core/modules/forum/tests/src/Functional/ForumIndexUpdateTest.php:31
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

and

1) Drupal\Tests\forum\Kernel\ForumIndexTest::testForumIndexIndex
Failed asserting that false is true.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/modules/forum/tests/src/Kernel/ForumIndexTest.php:49
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Reverted this

I could be wrong, but I don't see the message about isolation level/PKs in 10.0.x so perhaps this is fixed? I.e the issue doesn't exist in 10.0

Checking Drupal commit history, this error message was introduced in 10.1, thus not being shown in 10.0, although the problem itself was present there already

Left a comment on the MR - think we need to run a query to remove duplicates before adding the index. Not sure if this is related to the failures but it needs doing anyway.

That will also make the update more complex than it currently is, so I think for 10.0.x we should suppress the warning just for forum_index rather than fixing it there, then the update will run in 10.1.

I'm confused, the test passes on pgsql and sqlite (for 11.x)

The UI is a disaster zone when multiple MRs are on the same core-dev-branch, but if I look at this snippet from the dispatcher log of the sqllite passing test:

00:01:27.345 Creating list of files to check by comparing branch to 11.x
00:01:36.012 
1/1 ./modules/forum/forum.install 441.20ms
00:01:36.455 CSpell: Files checked: 1, Issues found: 0 in 0 files

I think (the closed) MR!4162, and not the needed and probably expected from the UI MR!4278 was tested and passed.

Updated both branches to use a DB agnostic approach

Seemed to cause a failure for pgsql and sqlite

Fixed the other test too

Looks good to me.
The testbot passes for all 3 databases.
Back to RTBC.

Sorry still think we need to do #55 here too.

Please review.

Is there any workaround that we can use until this gets fixed in the core? This issue is holding back us from updating to Drupal 10.1

The patch from comment 60 should be fine to apply.

Here's the push from #60 as a patch

Thanks, will try it out!

This needs a fix in the 10.1 release asap. Uninstalling the module failed, saying that the content had to be deleted, but there is no forum content! I did delete the forums.
So forum, a very important feature of Drupal, is dead.

Given #60, is this needs review, or still needs work?

Seems there are still open threads in MR 4279

I have tested the patch from #60 (#68) on copies of some of our websites and it seems to work correctly. Hoping the remaining issues get fixed and this gets into 10.1.x. Forum functionality works and I did not notice anything problematic logged.

As this is holding back the update of many of the websites, could someone with more knowledge on this confirm whether we can use 10.1.x without this patch applied and ignore the status page error, or are there other consequences?

Addressed the chance of duplicates in latest push with a new fixture and additional update hook and test coverage

The remark from @catch has been addressed.
The testbot is green for all 3 by core supported databases.
Back to RTBC.

Its a lot of code for "just" adding a primary key.

Very glad to see this in RTBC :)

More comments on the MR..

At first I thought we could use ->limit with a delete query, but we can't so we have to delete all the rows that are duplicates because there's no way with the delete query to only delete the duplicates.

So I'm doing that, then keeping track of the nids we need to recreate via state and looping over those in a post update hook to add back the missing values.

Ready for review again

That looks better now.

Only other idea I had to save the rebuild would be selecting the value of the duplicate rows, deleting it, then writing one row back, but given the duplicate rows it's likely some of the data is inaccurate anyway, so better to have the more complete implementation. Back to RTBC.

Couple of small comments on the MR.

Made those changes, nice one re the use of progress 👌

All points of @alexpott have been addressed.
Back to RTBC.

The issue summary is very confusing here; there are two MRs. The one that I would guess is for 11.x says it is closed -- which usually makes them disappear from the UI -- and the 10.1.x one appears to be failing PHPCS.

Could we please:

1. Hide any out-of-date patches
2. Close any out-of-date MRs
3. Get tests passing on valid, current MRs

Thanks! Tagging for issue summary update.

Removed patches, closed 10.1.x MR

Updated issue summary.

Failing test in the pipeline looked to be random JS failure in throbber.

Reran.

Back to NR

For the record MR is 4278 for review.

I think this is still broken on postgres. When I run locally on postgres I see the following error:

The update failed with the following message: "Failed: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42703]: Undefined column: 7 ERROR: column "count" does not exist LINE 5: HAVING (count > 1) ^: SELECT "fi"."nid" AS "nid", "fi"."tid" AS "tid", count(*) AS "count" FROM "test61798717forum_index" "fi" GROUP BY "nid", "tid" HAVING (count > 1); Array ( ) in forum_update_10101() (line 220 of /Volumes/dev/sites/drupal8alt.dev/core/modules/forum/forum.install)."
 /Volumes/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/UpdatePathTestTrait.php:59
 /Volumes/dev/sites/drupal8alt.dev/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:203
 /Volumes/dev/sites/drupal8alt.dev/core/modules/forum/tests/src/Functional/ForumIndexUpdateTest.php:41

I think you could fix this by doing

diff --git a/core/modules/forum/forum.install b/core/modules/forum/forum.install
index 9f5c16a0b14..41f21540599 100644
--- a/core/modules/forum/forum.install
+++ b/core/modules/forum/forum.install
@@ -215,8 +215,7 @@ function forum_update_10101(&$sandbox = NULL): PluralTranslatableMarkup {
     ->fields('fi', ['nid', 'tid'])
     ->groupBy('nid')
     ->groupBy('tid');
-  $alias = $query->addExpression('count(*)', 'count');
-  $query->having(sprintf('%s > 1', $alias));
+  $query->having('COUNT(*) > 1');
   $results = $query->execute();
   $nids_to_rebuild = [];
   foreach ($results as $row) {

Instead.

i am not getting such error

Greetings,

Installed D10 all good.
Drupal Version 10.1.5
Web Server Apache/2.4.52 (Ubuntu)
PHP Version 8.2.11
Database Version 8.0.34-0ubuntu0.22.04.1

Did this
To change the database transaction isolation level to "READ COMMITTED" add the following to the database connection array:

'init_commands' => [
'isolation_level' => 'SET SESSION tx_isolation=\'READ-COMMITTED\'',
],

Then activated the forum and got the error as described in the first post.

FYI

Jo

Nice nice, gitlab ci had the same fail as reported by @alexpott

https://git.drupalcode.org/project/drupal/-/pipelines/27221/test_report

Drupal\Tests\help\Functional\AddPermissionsUpdateTest::testUpdate
The update failed with the following message: "Failed: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42703]: Undefined column: 7 ERROR: column "count" does not exist LINE 5: HAVING (count > 1) ^: SELECT "fi"."nid" AS "nid", "fi"."tid" AS "tid", count(*) AS "count" FROM "test39549953forum_index" "fi" GROUP BY "nid", "tid" HAVING (count > 1); Array ( ) in forum_update_10101() (line 220 of /builds/project/drupal/core/modules/forum/forum.install)."

Pushed a fix for that and manually queued tests on pgsql/sqlite

Seems there are fetch errors in the MR?

Passing in gitlab

Any concern this ran on 8.1 sqlite 3 vs 8.2?

I kicked off extra runs on various environments, can't have too many environment runs on this issue.

All the test runs seemed to have passed.

Just upgrade to Drupal 10 and started to get similar errors.

The recommended level for Drupal is "READ COMMITTED". For this to work correctly, all tables must have a primary key. The following table(s) do not have a primary key: taxonomy_term_field_data_delme, taxonomy_term_hierarchy_delme, url_alias_20180508_19, url_alias_20180509_11, url_alias_20180509_12, url_alias_20180510_14, url_alias_20180510_18, url_alias_20180511_11, url_alias_20180511_17, url_alias_20180514_12, url_alias_20180515_11, url_alias_20180515_18, url_alias_20180516_19, url_alias_20180517_11, url_alias_20180518_12, url_alias_20180521_12, url_alias_20180522_13, url_alias_20180523_12, url_alias_20180604.

No issues on 9.5.11 and this patch https://www.drupal.org/project/drupal/issues/1650930

Drupal 10.1.6
PHP 8.1
MySQL 5.7.29

I think we need to test the batching in forum_post_update_recreate_forum_index_rows(). I think drupal-10.1.0.empty.testing.forum.gz needs contain more than 1 duplicate row and we need to run the update with the entity_update_batch_size setting set to 1.

The forum seems to work on 10.1.6 despite the error. Since it is an error, is the current advice to not use the forum with 10.1 until this issue is fixed? Is it ok to use the patch from comment 68 to get around the error? Any advice on what to do before this is fixed in 11 would be appreciated. Thanks!

The patch from the MR (add .diff to the MR URL) is the best option.
Please report back any issues you have.
I'll try to address the latest feedback today

@larowlan, thanks for the heads up on ".diff" on the MR URL. I hadn't done that before so I used the following url for the patch, hopefully that was what you meant:

https://git.drupalcode.org/project/drupal/-/merge_requests/4278.diff

I applied the patch and there were no errors, then I ran the database updates and the three updates ran with no errors (there were no duplicates found on update 10101). The error message no longer appears on the status page. Then I did some basic testing, adding forum topics, comments, editing and deleting and everything seemed to work, but like I mentioned everything seemed to work without the patch. Looked at the Drupal logs and don't see any errors or warnings.

I'm running Drupal 10.1.6, Apache/2.4.38 (Debian), PHP 8.1.14, mysql 8.0.33

Do you suggest I proceed with that MR diff patch for production?

Thanks!

@mkimmet I think it is safe - the remaining work is about adding more testing to the update path.

There is one scenario that might trip you up, if in the process of expanding the tests we find an issue and have to add an additional update hook, that might conflict with your site that has run the old update hooks. If that's the case I'll comment here and can help you resolve that (there are workarounds). It feels unlikely, but I'd be remiss if I didn't flag the possibility.

Thank you for reporting back your results in testing the patch, adding issue credit for that as it is valuable.

One thing to highlight, if you're using https://github.com/cweagans/composer-patches to apply the patch, you should not reference the MR URL directly - instead you should download the file from https://git.drupalcode.org/project/drupal/-/merge_requests/4278.diff, save it in your codebase and apply it using a relative path.

For example if you save the file into a PATCHES folder at the root of your project, in your composer patches entry you would refer to it using ./PATCHES/path-to-the-file.patch.

Anyone can push to a MR and there is the risk of a supply chain attack if someone pushed something malicious and your build system is blindly fetching the current diff.

Updated the MR, will queue the other DB engines, but they pass locally

The patch from MR works for me

Seems the MR failed SqlLite and postgreSql

Reran the failing jobs, looks random

Ah gotcha

As described in #105, I downloaded the patch from the MR, saved it to a file, then tried applying it using https://github.com/cweagans/composer-patches, but that failed. The patch in #60 did work. I was using Drupal 10.1.6 when testing.

Using a checkout of core's 10.1 branch, I applied the patch from the MR manually, and it produced the following output:

18:53 $ git apply -v 4278.diff
4278.diff:145: trailing whitespace.
l#Y¢lE�, 
4278.diff:176: trailing whitespace.
Ȕ��4�k���"x��G���t\|[�K8Ven��+^#[X�e~�+���:���i��6OW��B3G�8e���ޱ�3xҾp��j��Tz��
4278.diff:214: trailing whitespace.

��`�(��!�MC����#��}�n��nBR ���-5�������U/�����4��jcc��@/��Jv���I:���b�"uI_칌�!���߇�r
                                                                                    |ı��k�'n�-�.�g�P|��i�Z}k5���$�C8hB��{
4278.diff:221: trailing whitespace.
���3$h��* �c��)��F�%�O��5�E
4278.diff:238: trailing whitespace.
}m���D�Ok���)���ӥ1��"�#�1MT�uJ�y)}����'��j{�0[�_���'��]�PLT<�L���E}��p:
Checking patch core/modules/forum/forum.install...
Checking patch core/modules/forum/forum.post_update.php...
Checking patch core/modules/forum/tests/fixtures/update/drupal-10.1.0.empty.testing.forum.gz...
Checking patch core/modules/forum/tests/src/Functional/ForumIndexUpdateTest.php...
Checking patch core/modules/forum/tests/src/Kernel/ForumIndexTest.php...
Applied patch core/modules/forum/forum.install cleanly.
Applied patch core/modules/forum/forum.post_update.php cleanly.
Applied patch core/modules/forum/tests/fixtures/update/drupal-10.1.0.empty.testing.forum.gz cleanly.
Applied patch core/modules/forum/tests/src/Functional/ForumIndexUpdateTest.php cleanly.
Applied patch core/modules/forum/tests/src/Kernel/ForumIndexTest.php cleanly.
warning: squelched 1 whitespace error
warning: 6 lines add whitespace errors.

Could this be an issue with https://github.com/cweagans/composer-patches not handling the test fixtures properly?

You need to use git apply --binary when there are binary files

Repeating my comment from #101

I think we need to test the batching in forum_post_update_recreate_forum_index_rows(). I think drupal-10.1.0.empty.testing.forum.gz needs contain more than 1 duplicate row and we need to run the update with the entity_update_batch_size setting set to 1.

That was added in https://git.drupalcode.org/project/drupal/-/merge_requests/4278/diffs?co..., unless I misunderstood what is being asked

Bah... missed it. Thanks @larowlan

Committed f00184d and pushed to 11.x. Thanks!

Discussed backporting this to 10.2.x with @catch and we agreed to do it because a user can not get around the warning and it avoids "major release crossover hell".

Yeah, thanks for 10.2.x !

I'm having the same problem with Drupal 10.1.6. So just to check, this will be fixed in later versions 10.2 and after? Is it OK to continue to use the site until then, or do I need to disable the Forum module for now?
Thanks.

Yes, you're fine to keep running the patch/MR diff here until 10.2 is released.

@larowlan - sorry, which patch / MR? The one in #68 or something else? Nvm this question - I see the answer at #105.

Can I just wait until 10.2 comes out, or will that break something?
Thanks.

This was the MR https://git.drupalcode.org/project/drupal/-/merge_requests/4278

Thank you. I applied the patch and it fixed the error.

Posting the link for how to patch with composer here in case it's of use to others. https://www.drupal.org/docs/develop/using-composer/manage-dependencies#p...

Just adding a 10.1 patch for composer.