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

Feature database and table count limitation issue56347 #64781

Conversation

XuJia0210
Copy link
Contributor

@XuJia0210 XuJia0210 commented Jun 4, 2024

issue: #56347

Changelog category (leave one):

  • New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Add server settings max_table_num_to_throw and max_database_num_to_throw to limit the number of databases or tables on CREATE queries.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

CI Settings (Only check the boxes if you know what you are doing):

  • Allow: All Required Checks
  • Allow: Stateless tests
  • Allow: Stateful tests
  • Allow: Integration Tests
  • Allow: Performance tests
  • Allow: All NOT Required Checks
  • Allow: batch 1, 2 for multi-batch jobs
  • Allow: batch 3, 4, 5, 6 for multi-batch jobs

  • Exclude: Style check
  • Exclude: Fast test
  • Exclude: Integration Tests
  • Exclude: Stateless tests
  • Exclude: Stateful tests
  • Exclude: Performance tests
  • Exclude: All with ASAN
  • Exclude: All with Aarch64
  • Exclude: All with TSAN, MSAN, UBSAN, Coverage

  • Do not test
  • Upload binaries for special builds
  • Disable merge-commit
  • Disable CI cache

@CLAassistant
Copy link

CLAassistant commented Jun 4, 2024

CLA assistant check
All committers have signed the CLA.

@robot-ch-test-poll robot-ch-test-poll added the pr-feature Pull request with new product feature label Jun 4, 2024
@robot-ch-test-poll
Copy link
Contributor

robot-ch-test-poll commented Jun 4, 2024

This is an automated comment for commit c32b97d with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check nameDescriptionStatus
A SyncIf it fails, ask a maintainer for help⏳ pending
CI runningA meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR❌ failure
Integration testsThe integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests❌ failure
Mergeable CheckChecks if all other necessary checks are successful⏳ pending
Successful checks
Check nameDescriptionStatus
Flaky testsChecks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integration tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc✅ success
PR CheckChecks correctness of the PR's body✅ success
Style checkRuns a set of checks to keep the code style clean. If some of tests failed, see the related log from the report✅ success

@kssenii kssenii added the can be tested Allows running workflows for external contributors label Jun 4, 2024
{
size_t db_count = DatabaseCatalog::instance().getDatabases().size();
// there's an invisible system database _temporary_and_external_tables, so we need to subtract 1
if (db_count > 0)
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to check that this hidden system DB is actually present && DatabaseCatalog::instance().isDatabaseExist(DatabaseCatalog::TEMPORARY_DATABASE)

@hanfei1991 hanfei1991 self-assigned this Jun 6, 2024
# Generate config.xml
CONFIG_FILE="$TEMP_DIR/config.xml"
cat > "$CONFIG_FILE" <<EOL
<yandex>
Copy link
Member

Choose a reason for hiding this comment

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

  1. We have left yandex :)
  2. Don't do this in stateless test :( we write integration tests to test special config

@@ -0,0 +1,21 @@
create database db1;
Copy link
Member

Choose a reason for hiding this comment

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

Don't do this .... we only have .sql and .reference in 0_stateless directory

@hanfei1991
Copy link
Member

I write an integration test to replace original one. Feel free to amend/revert if you do not agree :)

I have another question, why we don't care about system tables for tables limit, but have to care system and other default databases for limit? We can make the behaviour unified: only care about database/table which users create.

@@ -138,6 +145,18 @@ BlockIO InterpreterCreateQuery::createDatabase(ASTCreateQuery & create)
throw Exception(ErrorCodes::DATABASE_ALREADY_EXISTS, "Database {} already exists.", database_name);
}

if (auto max_db = getContext()->getGlobalContext()->getServerSettings().max_database_num_to_throw; max_db > 0)
Copy link
Member

Choose a reason for hiding this comment

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

We can write two lines, don't have to be tooo brief

@@ -1537,6 +1556,16 @@ bool InterpreterCreateQuery::doCreateTable(ASTCreateQuery & create,
}
}

if (UInt64 max_table = getContext()->getGlobalContext()->getServerSettings().max_table_num_to_throw; max_table > 0)
Copy link
Member

Choose a reason for hiding this comment

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

ditto

UInt64 table_count = CurrentMetrics::get(CurrentMetrics::AttachedTable);
if (table_count >= max_table)
throw Exception(ErrorCodes::TOO_MANY_TABLES,
"Too many tables in the system. Current is {}, limit is {}. "
Copy link
Member

@hanfei1991 hanfei1991 Jun 10, 2024

Choose a reason for hiding this comment

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

How about using one sentence:
Too many tables in ClickHouse. The limit (setting 'max_table_num_to_throw') is set to {}

Same for databases.

Besides, do not write period at the end.

auto-merge was automatically disabled June 14, 2024 00:22

Head branch was pushed to by a user without write access

@XuJia0210 XuJia0210 closed this Jun 14, 2024
auto-merge was automatically disabled June 14, 2024 00:22

Pull request was closed

@XuJia0210 XuJia0210 force-pushed the feature_database_and_table_count_limitation_issue56347 branch from 8112250 to 594a0e9 Compare June 14, 2024 00:22
@XuJia0210 XuJia0210 reopened this Jun 14, 2024
@hanfei1991 hanfei1991 added this pull request to the merge queue Jun 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 19, 2024
@hanfei1991 hanfei1991 added this pull request to the merge queue Jun 19, 2024
Merged via the queue into ClickHouse:master with commit 5eca8db Jun 19, 2024
71 of 74 checks passed
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can be tested Allows running workflows for external contributors pr-feature Pull request with new product feature pr-synced-to-cloud The PR is synced to the cloud repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants