Skip to content

Commit c663e54

Browse files
snaurysnaury
authored
25-3-1: Fix schemeshard assertion when building index over a legacy table (#27740)
2 parents 0c4ca8f + 7326f33 commit c663e54

File tree

3 files changed

+183
-21
lines changed

3 files changed

+183
-21
lines changed

‎ydb/core/tx/schemeshard/schemeshard__operation_create_table.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,10 @@ void ApplyPartitioning(TTxId txId,
159159
}
160160

161161
std::optional<TString> InferDefaultPoolKind(const TStoragePools& storagePools, const NKikimrSchemeOp::TPartitionConfig& changes) {
162+
if (changes.HasChannelProfileId()) {
163+
return std::nullopt;
164+
}
165+
162166
// Refuse to infer the default pool kind if any column family in the requested changes has a legacy Storage parameter
163167
for (const auto& changesFamily : changes.GetColumnFamilies()) {
164168
if (changesFamily.HasStorage()) {

‎ydb/core/tx/schemeshard/schemeshard_info_types.cpp‎

Lines changed: 20 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1393,26 +1393,6 @@ bool TPartitionConfigMerger::VerifyAlterParams(
13931393
return false;
13941394
}
13951395

1396-
if (dstConfig.HasChannelProfileId()) {
1397-
for (const auto& family : dstConfig.GetColumnFamilies()) {
1398-
if (family.HasStorageConfig()) {
1399-
errDescr = TStringBuilder()
1400-
<< "Migration from profile id by storage config is not allowed, was "
1401-
<< srcConfig.GetChannelProfileId() << ", asks storage config";
1402-
return false;
1403-
}
1404-
}
1405-
1406-
if (srcConfig.GetChannelProfileId() != dstConfig.GetChannelProfileId()) {
1407-
errDescr = TStringBuilder()
1408-
<< "Profile modification is not allowed, was "
1409-
<< srcConfig.GetChannelProfileId()
1410-
<< ", asks "
1411-
<< dstConfig.GetChannelProfileId();
1412-
return false;
1413-
}
1414-
}
1415-
14161396
const NKikimrSchemeOp::TStorageConfig* wasStorageConfig = nullptr;
14171397
for (const auto& family : srcConfig.GetColumnFamilies()) {
14181398
if (family.GetId() == 0 && family.HasStorageConfig()) {
@@ -1453,7 +1433,26 @@ bool TPartitionConfigMerger::VerifyAlterParams(
14531433
if (isStorageConfig) {
14541434
if (!wasStorageConfig) {
14551435
errDescr = TStringBuilder()
1456-
<< "Couldn't add storage configuration if it hasn't been set before";
1436+
<< "Cannot add storage config if it hasn't been set before";
1437+
return false;
1438+
}
1439+
}
1440+
1441+
if (dstConfig.HasChannelProfileId()) {
1442+
// Note: ChannelProfileId == 0 may have been implicit
1443+
if (dstConfig.GetChannelProfileId() != srcConfig.GetChannelProfileId()) {
1444+
errDescr = TStringBuilder()
1445+
<< "Profile modification is not allowed, was "
1446+
<< srcConfig.GetChannelProfileId()
1447+
<< ", asks "
1448+
<< dstConfig.GetChannelProfileId();
1449+
return false;
1450+
}
1451+
1452+
if (isStorageConfig && !srcConfig.HasChannelProfileId()) {
1453+
errDescr = TStringBuilder()
1454+
<< "Setting ChannelProfileId to " << dstConfig.GetChannelProfileId()
1455+
<< " for tables with storage config is not allowed";
14571456
return false;
14581457
}
14591458
}

‎ydb/core/tx/schemeshard/ut_base/ut_base.cpp‎

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12246,4 +12246,163 @@ Y_UNIT_TEST_SUITE(TSchemeShardTest) {
1224612246
}
1224712247
})", {TEvSchemeShard::EStatus::StatusInvalidParameter});
1224812248
}
12249+
12250+
Y_UNIT_TEST(DefaultStorageConfigTableWithChannelProfileIdBuildIndex) {
12251+
TTestBasicRuntime runtime;
12252+
TTestEnv env(runtime, TTestEnvOptions().NStoragePools(1));
12253+
ui64 txId = 100;
12254+
12255+
// Create a legacy table when there are no storage pools yet
12256+
TestCreateTable(runtime, ++txId, "/MyRoot", R"(
12257+
Name: "Table1"
12258+
Columns { Name: "key" Type: "Uint32" }
12259+
Columns { Name: "value" Type: "Uint32" }
12260+
KeyColumnNames: ["key"]
12261+
PartitionConfig {
12262+
ChannelProfileId: 0
12263+
}
12264+
)");
12265+
env.TestWaitNotification(runtime, txId);
12266+
12267+
// Add a single storage pool so new tables could have it inferred
12268+
TestAlterSubDomain(runtime, ++txId, "/", R"(
12269+
Name: "MyRoot"
12270+
StoragePools {
12271+
Name: "pool-1"
12272+
Kind: "pool-kind-1"
12273+
}
12274+
)");
12275+
env.TestWaitNotification(runtime, txId);
12276+
12277+
// Build index over a legacy table
12278+
TestBuildIndex(runtime, ++txId, TTestTxConfig::SchemeShard, "/MyRoot", "/MyRoot/Table1", "Index", {"value"});
12279+
env.TestWaitNotification(runtime, txId);
12280+
}
12281+
12282+
Y_UNIT_TEST(CannotAddChannelProfileIdToStorageConfigTable) {
12283+
TTestBasicRuntime runtime;
12284+
TTestEnv env(runtime, TTestEnvOptions().NStoragePools(1));
12285+
ui64 txId = 100;
12286+
12287+
// Add a single storage pool
12288+
TestAlterSubDomain(runtime, ++txId, "/", R"(
12289+
Name: "MyRoot"
12290+
StoragePools {
12291+
Name: "pool-1"
12292+
Kind: "pool-kind-1"
12293+
}
12294+
)");
12295+
env.TestWaitNotification(runtime, txId);
12296+
12297+
// Create a storage config table
12298+
TestCreateTable(runtime, ++txId, "/MyRoot", R"(
12299+
Name: "Table1"
12300+
Columns { Name: "key" Type: "Uint32" }
12301+
Columns { Name: "value" Type: "Uint32" }
12302+
KeyColumnNames: ["key"]
12303+
PartitionConfig {
12304+
ColumnFamilies {
12305+
Id: 0
12306+
StorageConfig {
12307+
SysLog {
12308+
PreferredPoolKind: "pool-kind-1"
12309+
}
12310+
Log {
12311+
PreferredPoolKind: "pool-kind-1"
12312+
}
12313+
Data {
12314+
PreferredPoolKind: "pool-kind-1"
12315+
}
12316+
}
12317+
}
12318+
}
12319+
)");
12320+
env.TestWaitNotification(runtime, txId);
12321+
12322+
// Specifying ChannelProfileId should be an error
12323+
TestAlterTable(runtime, ++txId, "/MyRoot", R"(
12324+
Name: "Table1"
12325+
PartitionConfig {
12326+
ChannelProfileId: 0
12327+
}
12328+
)", {TEvSchemeShard::EStatus::StatusInvalidParameter});
12329+
}
12330+
12331+
Y_UNIT_TEST(AlterMixedStorageConfigAndChannelProfileIdTable) {
12332+
TTestBasicRuntime runtime;
12333+
TTestEnv env(runtime, TTestEnvOptions().NStoragePools(1));
12334+
ui64 txId = 100;
12335+
12336+
// Add a single storage pool
12337+
TestAlterSubDomain(runtime, ++txId, "/", R"(
12338+
Name: "MyRoot"
12339+
StoragePools {
12340+
Name: "pool-1"
12341+
Kind: "pool-kind-1"
12342+
}
12343+
)");
12344+
env.TestWaitNotification(runtime, txId);
12345+
12346+
// Create a table that has both default family with both StorageConfig
12347+
// and ChannelProfileId specified. Historically there is no validation
12348+
// for such mixed tables (StorageConfig wins out).
12349+
TestCreateTable(runtime, ++txId, "/MyRoot", R"(
12350+
Name: "Table1"
12351+
Columns { Name: "key" Type: "Uint32" }
12352+
Columns { Name: "value" Type: "Uint32" }
12353+
KeyColumnNames: ["key"]
12354+
PartitionConfig {
12355+
ChannelProfileId: 0
12356+
ColumnFamilies {
12357+
Id: 0
12358+
StorageConfig {
12359+
SysLog {
12360+
PreferredPoolKind: "pool-kind-1"
12361+
}
12362+
Log {
12363+
PreferredPoolKind: "pool-kind-1"
12364+
}
12365+
Data {
12366+
PreferredPoolKind: "pool-kind-1"
12367+
}
12368+
}
12369+
}
12370+
}
12371+
)");
12372+
env.TestWaitNotification(runtime, txId);
12373+
12374+
// It should be possible to alter tables while specifying the same
12375+
// exact mixed settings partition config.
12376+
TestAlterTable(runtime, ++txId, "/MyRoot", R"(
12377+
Name: "Table1"
12378+
Columns { Name: "value2" Type: "Uint32" }
12379+
PartitionConfig {
12380+
ChannelProfileId: 0
12381+
ColumnFamilies {
12382+
Id: 0
12383+
StorageConfig {
12384+
SysLog {
12385+
PreferredPoolKind: "pool-kind-1"
12386+
}
12387+
Log {
12388+
PreferredPoolKind: "pool-kind-1"
12389+
}
12390+
Data {
12391+
PreferredPoolKind: "pool-kind-1"
12392+
}
12393+
}
12394+
}
12395+
}
12396+
)");
12397+
env.TestWaitNotification(runtime, txId);
12398+
12399+
// Changing ChannelProfileId should not be allowed
12400+
TestAlterTable(runtime, ++txId, "/MyRoot", R"(
12401+
Name: "Table1"
12402+
Columns { Name: "value2" Type: "Uint32" }
12403+
PartitionConfig {
12404+
ChannelProfileId: 1
12405+
}
12406+
)", {TEvSchemeShard::EStatus::StatusInvalidParameter});
12407+
}
1224912408
}

0 commit comments

Comments
 (0)