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

ALTER INDEX to change partitioning settings of indexImplTable #4354

Merged
merged 4 commits into from
May 23, 2024

Conversation

jepett0
Copy link
Collaborator

@jepett0 jepett0 commented May 7, 2024

List of existing tickets on the topic: https://github.com/orgs/ydb-platform/projects/31/views/3?pane=issue&itemId=55226292
Requirements: https://st.yandex-team.ru/KIKIMR-14280

You can read the whole story in the RFC, but the gist of it is provided below.

Previously, there was no available instrument to alter partitioning settings of an index table in spite of the often arising need to do so. It could only be done by superusers (i.e. users listed in AdministrationAllowedSIDs of the cluster; usually just the YDB staff) with either a public API request, or by sending a private SchemeShard's ModifyScheme protobuf. No YQL syntax was available for use even for the superusers (indexImplTable is hidden and cannot be a target of ALTER TABLE request).

We decided to provide users with a way to conveniently alter partitioning settings of an index table. It seems that the most needed options is to set AUTO_PARTITIONING_MIN_PARTITIONS_COUNT to a higher value to prepare for the future load on the index.

Syntax

ALTER TABLE table_name ALTER INDEX index_name SET partitioning_setting_name value
ALTER TABLE table_name ALTER INDEX index_name SET (partitioning_setting_name_1 = value_1, ...)

table_name - the name of the table, whose index is to be altered

index_name - the name of the index to be altered

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

@jepett0 jepett0 force-pushed the SchemeShard.alter_index.1 branch from 07617fb to e49fbcc Compare May 13, 2024 21:43

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

@jepett0 jepett0 force-pushed the SchemeShard.alter_index.1 branch from e49fbcc to 872f675 Compare May 16, 2024 18:14

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

@jepett0 jepett0 force-pushed the SchemeShard.alter_index.1 branch from 872f675 to 505535e Compare May 17, 2024 07:44

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

@jepett0 jepett0 marked this pull request as ready for review May 17, 2024 11:08
@jepett0 jepett0 requested a review from a team as a code owner May 17, 2024 11:08
gridnevvvit
gridnevvvit previously approved these changes May 19, 2024
Copy link

github-actions bot commented May 20, 2024

2024-05-20 11:19:48 UTC Pre-commit check for a0fb41e has started.
2024-05-20 11:19:50 UTC Build linux-x86_64-relwithdebinfo is running...
🟢 2024-05-20 11:41:47 UTC Build successful.
2024-05-20 11:43:37 UTC Tests are running...
🔴 2024-05-20 13:41:27 UTC Some tests failed, follow the links below.

Test history

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
72420 59712 0 6 12691 11

Copy link

github-actions bot commented May 20, 2024

2024-05-20 11:21:03 UTC Pre-commit check for a0fb41e has started.
2024-05-20 11:21:06 UTC Build linux-x86_64-release-asan is running...
🟢 2024-05-20 11:43:24 UTC Build successful.
2024-05-20 11:45:15 UTC Tests are running...
🔴 2024-05-20 13:47:23 UTC Some tests failed, follow the links below.

Test history

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
13479 13227 0 60 167 25

Copy link

github-actions bot commented May 20, 2024

2024-05-20 11:21:09 UTC Pre-commit check for a0fb41e has started.
2024-05-20 11:21:11 UTC Build linux-x86_64-release-clang14 is running...
🟢 2024-05-20 11:43:31 UTC Build successful.

@jepett0 jepett0 requested a review from gridnevvvit May 20, 2024 14:21
Comment on lines +739 to +741
&& (!CheckAllowedFields(alter, {"Name", "PartitionConfig"})
|| (alter.HasPartitionConfig()
&& !CheckAllowedFields(alter.GetPartitionConfig(), {"PartitioningPolicy"})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This condition is hard to read and comprehend.

Copy link
Collaborator Author

@jepett0 jepett0 May 23, 2024

Choose a reason for hiding this comment

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

Will try to simplify it in a follow-up PR

Comment on lines -10510 to -10511
auto wellCookedToken = NACLib::TUserToken(TVector<TString>{"not-a-root@builtin"});
request->Record.SetUserToken(wellCookedToken.SerializeAsString());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why cases with requests with explicit tokens are gone? Check for admin token in alter table is still there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the check for superuser is still present in CreateConsistentAlterTable, but the purpose has changed. Currently, every user with AlterScheme permission can alter PartitionConfig.PartitioningPolicy of indexImplTable and superusers can alter any setting of indexImplTable (like they did before).

This particular test, in my opinion, is focused more on the effects of altering PartitionConfig of indexImplTable, then on the permissions of superusers. I think the best solution would be to add another unit test to show the difference between superusers and regular users. In fact, I have already added such a unit test in KQP, but not in SchemeShard.

I will add a similar test in SchemeShard in a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants