Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to stabilize test 02346_full_text_search.sql #46344

Merged
merged 2 commits into from
Feb 15, 2023

Conversation

rschu1ze
Copy link
Member

Failure, e.g. https://s3.amazonaws.com/clickhouse-test-reports/0/f3fa3d868cb9957eeca6b5d5fd653234f5db5b45/stateless_tests__release__databaseordinary_.html :

"Cannot quickly remove directory /var/lib/clickhouse/data/test_ggwgonnk/tab/delete_tmp_all_1_1_0 by removing files; fallback to recursive removal. Reason: Code: 566. DB::ErrnoException: Cannot rmdir /var/lib/clickhouse/data/test_ggwgonnk/tab/delete_tmp_all_1_1_0, errno: 39, strerror: Directory not empty. (CANNOT_RMDIR) (version 23.2.1.1427 (official build))"

The issue did not reproduce in a local debug or release builds.

I noticed that it happens only with "DatabaseOrdinary" test profiles: https://play.clickhouse.com/play?user=play#U0VMRUNUIGNoZWNrX3N0YXJ0X3RpbWUsIGNoZWNrX25hbWUsIHRlc3RfbmFtZSwgcmVwb3J0X3VybApGUk9NIGNoZWNrcwpXSEVSRSBjaGVja19zdGFydF90aW1lID49IG5vdygpIC0gSU5URVJWQUwgOTYgSE9VUgogICAgQU5EIHB1bGxfcmVxdWVzdF9udW1iZXIgPSAwCiAgICBBTkQgdGVzdF9zdGF0dXMgIT0gJ1NLSVBQRUQnCiAgICBBTkQgdGVzdF9zdGF0dXMgTElLRSAnRiUnCiAgICBBTkQgY2hlY2tfc3RhdHVzICE9ICdzdWNjZXNzJwogICAgQU5EIHBvc2l0aW9uKHRlc3RfbmFtZSwgJzAyMzQ2X2Z1bGxfdGV4dF9zZWFyY2gnKSA+IDAKT1JERVIgQlkgY2hlY2tfc3RhcnRfdGltZQ==

"Ordinary" databases were deprecated in ca. 2020 and nowadays can no longer be created unless a special setting is on.

There is actually a fallback path (recursive removal) and I am not sure why this is logged as "error" instead of "warning".

For now disabling 02346_full_text_search for ordinary databases.

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Feb 13, 2023
@devcrafter devcrafter self-assigned this Feb 13, 2023
@tavplubix
Copy link
Member

Should we disable this feature with Ordinary engine then?

@tavplubix
Copy link
Member

Hm, probably the same issue happens with Atomic databases as well, but we don't see it because the error is logged by a background thread, so clickouse-test cannot notice it.

@tavplubix tavplubix self-assigned this Feb 13, 2023
Copy link
Member

@tavplubix tavplubix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need more info about the issue, see the comments above

@tavplubix
Copy link
Member

Probably some file related to indexes was not added to MergeTreeDataPartChecksums::files for some reason

Failure, e.g. https://s3.amazonaws.com/clickhouse-test-reports/0/f3fa3d868cb9957eeca6b5d5fd653234f5db5b45/stateless_tests__release__databaseordinary_.html :

    "Cannot quickly remove directory /var/lib/clickhouse/data/test_ggwgonnk/tab/delete_tmp_all_1_1_0 by removing files; fallback to recursive removal. Reason: Code: 566. DB::ErrnoException: Cannot rmdir /var/lib/clickhouse/data/test_ggwgonnk/tab/delete_tmp_all_1_1_0, errno: 39, strerror: Directory not empty. (CANNOT_RMDIR) (version 23.2.1.1427 (official build))"

The issue did not reproduce in a local debug or release builds.

Then noticed that it happens only with "DatabaseOrdinary" test profiles: https://play.clickhouse.com/play?user=play#U0VMRUNUIGNoZWNrX3N0YXJ0X3RpbWUsIGNoZWNrX25hbWUsIHRlc3RfbmFtZSwgcmVwb3J0X3VybApGUk9NIGNoZWNrcwpXSEVSRSBjaGVja19zdGFydF90aW1lID49IG5vdygpIC0gSU5URVJWQUwgOTYgSE9VUgogICAgQU5EIHB1bGxfcmVxdWVzdF9udW1iZXIgPSAwCiAgICBBTkQgdGVzdF9zdGF0dXMgIT0gJ1NLSVBQRUQnCiAgICBBTkQgdGVzdF9zdGF0dXMgTElLRSAnRiUnCiAgICBBTkQgY2hlY2tfc3RhdHVzICE9ICdzdWNjZXNzJwogICAgQU5EIHBvc2l0aW9uKHRlc3RfbmFtZSwgJzAyMzQ2X2Z1bGxfdGV4dF9zZWFyY2gnKSA+IDAKT1JERVIgQlkgY2hlY2tfc3RhcnRfdGltZQ==

"Ordinary" databases were deprecated in ca. 2020 and nowadays can no
longer be created unless a special setting is on.

There is actually a fallback path (recursive removal) so it is arguable
why this is logged as error.

For now disabling 02346_full_text_search for ordinary databases.
@rschu1ze
Copy link
Member Author

Hmm, good points. Yes, the inverted index adds extra files to the persistence. Let me try a real fix...

@rschu1ze rschu1ze force-pushed the rs/stabilize-full_text_seaerch_test branch from e14db0a to 95c50b8 Compare February 14, 2023 12:33
@rschu1ze
Copy link
Member Author

The immediate problem is solved in a bit hacky way. I did not find a more natural place where the checksums are populated.

Copy link
Member

@tavplubix tavplubix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay for a hotfix

@tavplubix
Copy link
Member

where the checksums are populated.

See loadColumnsChecksumsIndexes, fillSkipIndicesChecksums, and places like finalizeMutatedPart

@rschu1ze rschu1ze merged commit ec33204 into master Feb 15, 2023
@rschu1ze rschu1ze deleted the rs/stabilize-full_text_seaerch_test branch February 15, 2023 09:47
rschu1ze added a commit that referenced this pull request Feb 26, 2023
The (experimental) inverted index writes/reads files different from the
standard files written by the other skip indexes. The original problem
was that with database engine "ordinary", DROP TABLE of a table with
inverted index finds unknown files in persistence and complains. The
same will happen with engine "atomic" but deferred. As a hotfix, the
error was silenced by explicitly adding the four files created in a
specific test to the deletion code.

This PR tries a cleaner solution where all needed files are provided via
the normal checksum structure. One drawback remains which is that the
affected files were written earlier and we don't have their checksums
available. Therefore, the inverted index is currently excluded from
CHECK TABLE.

Minimal repro:
  SET allow_experimental_inverted_index = 1;
  DROP TABLE IF EXISTS tab;
  CREATE TABLE tab(s String, INDEX af(s) TYPE inverted(2)) ENGINE = MergeTree() ORDER BY s;
  INSERT INTO tab VALUES ('Alick a01');
  CHECK TABLE tab;
  DROP TABLE IF EXISTS tab;

  run ./clickhouse-test with --db-engine Ordinary
rschu1ze added a commit that referenced this pull request Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-not-for-changelog This PR should not be mentioned in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants