From d93cf7b6d29339d0d4f244401ab7bdcefb84875d Mon Sep 17 00:00:00 2001 From: Igor Gaponenko Date: Thu, 19 Oct 2023 03:33:55 +0000 Subject: [PATCH 1/2] Minor refactoring of the Replication system configuration service Eliminated an ambiguity in the table properties 'isDirector' and 'isRefMatch'. In the older version of the table configuration class, these properties could be obtained explicitly via the corresponding data members or implicitly (via other members). That could result in an inconsistent state of a table definition. --- src/replica/ConfigAppBase.cc | 4 +- src/replica/ConfigDatabase.cc | 38 ++++------- src/replica/ConfigParserMySQL.cc | 8 +-- src/replica/ConfigTable.cc | 4 +- src/replica/ConfigTable.h | 8 ++- src/replica/ConfigTestApp.cc | 2 - src/replica/DirectorIndexJob.cc | 2 +- src/replica/HttpDirectorIndexModule.cc | 2 +- src/replica/HttpIngestModule.cc | 14 ++-- src/replica/HttpIngestTransModule.cc | 4 +- src/replica/HttpQservMonitorModule.cc | 2 +- src/replica/QservMgtServices.cc | 2 + src/replica/WorkerDirectorIndexRequest.cc | 2 +- src/replica/testConfiguration.cc | 78 +++++++++++------------ 14 files changed, 79 insertions(+), 91 deletions(-) diff --git a/src/replica/ConfigAppBase.cc b/src/replica/ConfigAppBase.cc index 5ffe98edda..9661a7ceb1 100644 --- a/src/replica/ConfigAppBase.cc +++ b/src/replica/ConfigAppBase.cc @@ -177,8 +177,8 @@ void ConfigAppBase::dumpDatabasesAsTable(string const& indent, string const& cap publishTime.push_back(to_string(database.publishTime)); tableName.push_back(table.name); isPartitioned.push_back(table.isPartitioned ? "yes" : "no"); - isDirector.push_back(table.isDirector ? "yes" : "no"); - isRefMatch.push_back(table.isRefMatch ? "yes" : "no"); + isDirector.push_back(table.isDirector() ? "yes" : "no"); + isRefMatch.push_back(table.isRefMatch() ? "yes" : "no"); directorTable.push_back(table.directorTable.databaseTableName()); directorKey.push_back(table.directorTable.primaryKeyColumn()); directorTable2.push_back(table.directorTable2.databaseTableName()); diff --git a/src/replica/ConfigDatabase.cc b/src/replica/ConfigDatabase.cc index eb520ee6b3..a197117a2c 100644 --- a/src/replica/ConfigDatabase.cc +++ b/src/replica/ConfigDatabase.cc @@ -92,8 +92,6 @@ DatabaseInfo DatabaseInfo::parse(json const& obj, map() != 0; table.latitudeColName = tableJson.at("latitude_key").get(); table.longitudeColName = tableJson.at("longitude_key").get(); - table.isDirector = table.isPartitioned && table.directorTable.tableName().empty(); - table.isRefMatch = table.isPartitioned && !table.directorTable2.tableName().empty(); } if (tableJson.count("columns") != 0) { json const& columns = tableJson.at("columns"); @@ -156,16 +154,16 @@ vector DatabaseInfo::partitionedTables() const { vector DatabaseInfo::directorTables() const { vector result; - for (auto&& itr : _tables) { - if (itr.second.isDirector) result.push_back(itr.first); + for (auto&& [name, table] : _tables) { + if (table.isDirector()) result.push_back(name); } return result; } vector DatabaseInfo::refMatchTables() const { vector result; - for (auto&& itr : _tables) { - if (itr.second.isRefMatch) result.push_back(itr.first); + for (auto&& [name, table] : _tables) { + if (table.isRefMatch()) result.push_back(name); } return result; } @@ -210,8 +208,8 @@ TableInfo DatabaseInfo::validate(map const& databases throwIf(!table.isPublished && (table.publishTime != 0), "the publish timestamp of the non-published table is not 0"); - bool const isRegularType = !table.isPartitioned && !(table.isDirector || table.isRefMatch); - bool const isPartitionedType = table.isPartitioned && !(table.isDirector && table.isRefMatch); + bool const isRegularType = !table.isPartitioned && !(table.isDirector() || table.isRefMatch()); + bool const isPartitionedType = table.isPartitioned && !(table.isDirector() && table.isRefMatch()); throwIfNot(isRegularType || isPartitionedType, "ambiguous table type definition"); if (table.isPartitioned) { @@ -219,7 +217,7 @@ TableInfo DatabaseInfo::validate(map const& databases // the table, depending on its declared type. map colDefs; - if (table.isDirector) { + if (table.isDirector()) { throwIfNot(table.directorTable.tableName().empty() && table.directorTable2.empty(), "the director table can't be the dependant of other director(s)"); @@ -241,7 +239,7 @@ TableInfo DatabaseInfo::validate(map const& databases // This column is required for the director tables to allow Qserv materialize // sub-chunks in the near-neighbour queries. colDefs.insert({"subChunkIdColName", lsst::qserv::SUB_CHUNK_COLUMN}); - } else if (table.isRefMatch) { + } else if (table.isRefMatch()) { throwIf(table.directorTable.empty() || table.directorTable2.empty(), "incomplete definition of the directors for the RefMatch table"); throwIf(table.directorTable == table.directorTable2, @@ -262,7 +260,7 @@ TableInfo DatabaseInfo::validate(map const& databases throwIfNot(database->tableExists(tableRef.tableName()), "non-existing director '" + tableRef.tableName() + "' referenced in the RefMatch definition"); - throwIfNot(database->findTable(tableRef.tableName()).isDirector, + throwIfNot(database->findTable(tableRef.tableName()).isDirector(), "table '" + tableRef.tableName() + "' referenced in the RefMatch definition isn't the director"); } @@ -294,7 +292,7 @@ TableInfo DatabaseInfo::validate(map const& databases " table spec of the dependent tables"); throwIfNot(tableExists(table.directorTable.tableName()), "non-existing director table referenced in the dependent table definition"); - throwIfNot(findTable(table.directorTable.tableName()).isDirector, + throwIfNot(findTable(table.directorTable.tableName()).isDirector(), "a table referenced in the dependent table definition isn't the director table"); throwIfNot(table.directorTable2.empty(), "the dependent table can't have the second director"); @@ -350,11 +348,11 @@ TableInfo DatabaseInfo::sanitize(TableInfo const& table_) const { table.publishTime = 0; } if (table.isPartitioned) { - if (table.isDirector != table.isRefMatch) { + if (table.isDirector() != table.isRefMatch()) { // For the known specialization of the partitioned table type sanitize // other attributes depending on the type. Note that such explicit // specialization always takes precedence. - if (table.isDirector) { + if (table.isDirector()) { table.directorTable = DirectorTableRef("", table.directorTable.primaryKeyColumn()); table.directorTable2 = DirectorTableRef(); table.flagColName = string(); @@ -363,7 +361,7 @@ TableInfo DatabaseInfo::sanitize(TableInfo const& table_) const { table.latitudeColName = string(); table.longitudeColName = string(); } - } else if (table.isDirector && table.isRefMatch) { + } else if (table.isDirector() && table.isRefMatch()) { // It's impossible to do anything here due to the explicitly // made table type ambiguity. ; @@ -371,30 +369,22 @@ TableInfo DatabaseInfo::sanitize(TableInfo const& table_) const { // If neither type flags were set then try deducing the table type based // on the presence of the director table columns. if (table.directorTable.tableName().empty()) { - table.isDirector = true; - table.isRefMatch = false; table.directorTable = DirectorTableRef("", table.directorTable.primaryKeyColumn()); table.directorTable2 = DirectorTableRef(); table.flagColName = string(); table.angSep = 0; } else { if (table.directorTable2.tableName().empty()) { - table.isDirector = false; - table.isRefMatch = false; table.directorTable2 = DirectorTableRef(); table.flagColName = string(); table.angSep = 0; } else { - table.isDirector = false; - table.isRefMatch = true; table.latitudeColName = string(); table.longitudeColName = string(); } } } } else { - table.isDirector = false; - table.isRefMatch = false; table.directorTable = DirectorTableRef(); table.directorTable2 = DirectorTableRef(); table.latitudeColName = string(); @@ -414,7 +404,7 @@ void DatabaseInfo::removeTable(string const& tableName) { "'."); } TableInfo& thisTableInfo = thisTableItr->second; - if (thisTableInfo.isDirector) { + if (thisTableInfo.isDirector()) { // Make sure no dependent tables exists for this director // among other partitioned tables. for (auto&& itr : _tables) { diff --git a/src/replica/ConfigParserMySQL.cc b/src/replica/ConfigParserMySQL.cc index 5dff17cb5c..d089a733a3 100644 --- a/src/replica/ConfigParserMySQL.cc +++ b/src/replica/ConfigParserMySQL.cc @@ -138,10 +138,6 @@ void ConfigParserMySQL::_parseDatabases() { table.latitudeColName = _parseParam("latitude_key"); table.longitudeColName = _parseParam("longitude_key"); table.isPartitioned = _parseParam("is_partitioned") != 0; - table.isDirector = table.isPartitioned && table.directorTable.tableName().empty() && - !table.directorTable.primaryKeyColumn().empty() && table.directorTable2.empty(); - table.isRefMatch = - table.isPartitioned && !table.directorTable.empty() && !table.directorTable2.empty(); tables.emplace_back(table); } @@ -163,10 +159,10 @@ void ConfigParserMySQL::_parseDatabases() { // This algorithm will enforce the referential integrity between the partitioned tables. // Pushing partitioned tables in the wrong order will fail the registration. for (auto&& table : tables) { - if (table.isDirector) _databases[table.database].addTable(_databases, table); + if (table.isDirector()) _databases[table.database].addTable(_databases, table); } for (auto&& table : tables) { - if (!table.isDirector) _databases[table.database].addTable(_databases, table); + if (!table.isDirector()) _databases[table.database].addTable(_databases, table); } } diff --git a/src/replica/ConfigTable.cc b/src/replica/ConfigTable.cc index b8bdae3e17..e9e86aee13 100644 --- a/src/replica/ConfigTable.cc +++ b/src/replica/ConfigTable.cc @@ -111,8 +111,8 @@ json TableInfo::toJson() const { result["create_time"] = createTime; result["publish_time"] = publishTime; result["is_partitioned"] = isPartitioned ? 1 : 0; - result["is_director"] = isDirector ? 1 : 0; - result["is_ref_match"] = isRefMatch ? 1 : 0; + result["is_director"] = isDirector() ? 1 : 0; + result["is_ref_match"] = isRefMatch() ? 1 : 0; result["director_table"] = directorTable.databaseTableName(); result["director_database_name"] = directorTable.databaseName(); result["director_table_name"] = directorTable.tableName(); diff --git a/src/replica/ConfigTable.h b/src/replica/ConfigTable.h index d6ab7d9e78..5116425580 100644 --- a/src/replica/ConfigTable.h +++ b/src/replica/ConfigTable.h @@ -122,11 +122,13 @@ class TableInfo { uint64_t createTime = 0; uint64_t publishTime = 0; - // The type of the table is determined by these flags. + // The type of the table is determined by these attributes. bool isPartitioned = false; - bool isDirector = false; - bool isRefMatch = false; + bool isDirector() const { return isPartitioned && directorTable.tableName().empty(); } + bool isRefMatch() const { + return isPartitioned && !directorTable.empty() && !directorTable2.tableName().empty(); + } /** * @brief The "director" table (if any). diff --git a/src/replica/ConfigTestApp.cc b/src/replica/ConfigTestApp.cc index 1881e0fe72..1c19eb1724 100644 --- a/src/replica/ConfigTestApp.cc +++ b/src/replica/ConfigTestApp.cc @@ -879,7 +879,6 @@ bool ConfigTestApp::_testTables() { table1.name = "director-1"; table1.database = database; table1.isPartitioned = true; - table1.isDirector = true; table1.directorTable = DirectorTableRef("", "objectId"); table1.uniquePrimaryKey = false; table1.latitudeColName = "decl"; @@ -905,7 +904,6 @@ bool ConfigTestApp::_testTables() { table2.name = "director-2"; table2.database = database; table2.isPartitioned = true; - table2.isDirector = true; table2.directorTable = DirectorTableRef("", "id"); table2.latitudeColName = "coord_decl"; table2.longitudeColName = "coord_ra"; diff --git a/src/replica/DirectorIndexJob.cc b/src/replica/DirectorIndexJob.cc index 2dfd915026..4dfbabce93 100644 --- a/src/replica/DirectorIndexJob.cc +++ b/src/replica/DirectorIndexJob.cc @@ -94,7 +94,7 @@ DirectorIndexJob::DirectorIndexJob(string const& databaseName, string const& dir _onFinish(onFinish) { try { _database = controller->serviceProvider()->config()->databaseInfo(databaseName); - if (!_database.findTable(directorTableName).isDirector) { + if (!_database.findTable(directorTableName).isDirector()) { throw runtime_error(context() + "::" + string(__func__) + " no such director table '" + directorTableName + "' in the database: '" + _database.name + "'."); } diff --git a/src/replica/HttpDirectorIndexModule.cc b/src/replica/HttpDirectorIndexModule.cc index 105299813b..22fa157c0d 100644 --- a/src/replica/HttpDirectorIndexModule.cc +++ b/src/replica/HttpDirectorIndexModule.cc @@ -80,7 +80,7 @@ json HttpDirectorIndexModule::_buildDirectorIndex() { auto const database = config->databaseInfo(databaseName); auto const table = database.findTable(directorTableName); - if (!table.isDirector) { + if (!table.isDirector()) { string const msg = "table '" + table.name + "' is not configured as a director table in database '" + database.name + "'"; throw HttpError(__func__, msg); diff --git a/src/replica/HttpIngestModule.cc b/src/replica/HttpIngestModule.cc index 6db13a0f3f..f2dd6563ab 100644 --- a/src/replica/HttpIngestModule.cc +++ b/src/replica/HttpIngestModule.cc @@ -621,7 +621,7 @@ json HttpIngestModule::_addTable() { // This operation can be vetoed by a catalog ingest workflow at the database // registration time. if (autoBuildDirectorIndex(database.name)) { - if (table.isDirector) { + if (table.isDirector()) { _createDirectorIndex(config->databaseInfo(database.name), table.name); } } @@ -678,7 +678,7 @@ json HttpIngestModule::_deleteTable() { bool const ifExists = true; conn->execute(g.dropTable(g.id(database.name, table.name), ifExists)); // Remove the director index (if any) - if (table.isDirector) { + if (table.isDirector()) { string const query = g.dropTable( g.id("qservMeta", directorIndexTableName(database.name, table.name)), ifExists); conn->execute(query); @@ -1180,7 +1180,7 @@ void HttpIngestModule::_publishDatabaseInMaster(DatabaseInfo const& database) co // Skip tables that have been published. if (table.isPublished) continue; if (!cssAccess->containsTable(database.name, table.name)) { - if (table.isRefMatch) { + if (table.isRefMatch()) { css::MatchTableParams const matchParams( table.directorTable.databaseTableName(), table.directorTable.primaryKeyColumn(), table.directorTable2.databaseTableName(), table.directorTable2.primaryKeyColumn(), @@ -1190,9 +1190,9 @@ void HttpIngestModule::_publishDatabaseInMaster(DatabaseInfo const& database) co // These parameters need to be set correctly for the 'director' and dependent // tables to avoid confusing Qserv query analyzer. Also note, that the 'overlap' // is set to be the same for all 'director' tables of the database family. - double const overlap = table.isDirector ? databaseFamilyInfo.overlap : 0; + double const overlap = table.isDirector() ? databaseFamilyInfo.overlap : 0; bool const isPartitioned = true; - bool const hasSubChunks = table.isDirector; + bool const hasSubChunks = table.isDirector(); css::PartTableParams const partParams(database.name, table.directorTable.tableName(), table.directorTable.primaryKeyColumn(), table.latitudeColName, table.longitudeColName, overlap, @@ -1297,7 +1297,7 @@ json HttpIngestModule::_buildEmptyChunksListImpl(string const& databaseName, boo void HttpIngestModule::_createDirectorIndex(DatabaseInfo const& database, string const& directorTableName) const { auto const& table = database.findTable(directorTableName); - if (!table.isDirector) { + if (!table.isDirector()) { throw logic_error("table '" + table.name + "' is not configured in database '" + database.name + "' as the director table"); } @@ -1355,7 +1355,7 @@ void HttpIngestModule::_createDirectorIndex(DatabaseInfo const& database, void HttpIngestModule::_consolidateDirectorIndex(DatabaseInfo const& database, string const& directorTableName) const { auto const table = database.findTable(directorTableName); - if (!table.isDirector) { + if (!table.isDirector()) { throw logic_error("table '" + table.name + "' is not configured in database '" + database.name + "' as the director table"); } diff --git a/src/replica/HttpIngestTransModule.cc b/src/replica/HttpIngestTransModule.cc index 3b128afa39..c8541e191a 100644 --- a/src/replica/HttpIngestTransModule.cc +++ b/src/replica/HttpIngestTransModule.cc @@ -485,7 +485,7 @@ void HttpIngestTransModule::_addPartitionToDirectorIndex(DatabaseInfo const& dat TransactionId transactionId, string const& directorTableName) const { auto const table = database.findTable(directorTableName); - if (!table.isDirector) { + if (!table.isDirector()) { throw logic_error("table '" + table.name + "' is not configured in database '" + database.name + "' as the director table"); } @@ -505,7 +505,7 @@ void HttpIngestTransModule::_removePartitionFromDirectorIndex(DatabaseInfo const TransactionId transactionId, string const& directorTableName) const { auto const table = database.findTable(directorTableName); - if (!table.isDirector) { + if (!table.isDirector()) { throw logic_error("table '" + table.name + "' is not configured in database '" + database.name + "' as the director table"); } diff --git a/src/replica/HttpQservMonitorModule.cc b/src/replica/HttpQservMonitorModule.cc index 2dbd8b8b2f..3a6d2c9fe9 100644 --- a/src/replica/HttpQservMonitorModule.cc +++ b/src/replica/HttpQservMonitorModule.cc @@ -759,7 +759,7 @@ json HttpQservMonitorModule::_css() { vector sharedScanTables; for (string const& tableName : database.tables()) { auto const table = database.findTable(tableName); - if (table.isPartitioned && !table.isRefMatch) { + if (table.isPartitioned && !table.isRefMatch()) { sharedScanTables.emplace_back(table.name); // Set the empty object as the default result for each table. resultSharedScan[familyName][database.name][table.name] = json::object(); diff --git a/src/replica/QservMgtServices.cc b/src/replica/QservMgtServices.cc index 74f4144165..7c28b3299e 100644 --- a/src/replica/QservMgtServices.cc +++ b/src/replica/QservMgtServices.cc @@ -358,7 +358,9 @@ GetConfigQservMgtRequest::Ptr QservMgtServices::config(std::string const& worker return request; } else { replica::Lock lock(_mtx, "QservMgtServices::" + string(__func__)); + auto const manager = shared_from_this(); + request = GetConfigQservMgtRequest::create( serviceProvider(), worker, [manager](QservMgtRequest::Ptr const& request) { manager->_finish(request->id()); }); diff --git a/src/replica/WorkerDirectorIndexRequest.cc b/src/replica/WorkerDirectorIndexRequest.cc index a80f0f5476..89b929fd3b 100644 --- a/src/replica/WorkerDirectorIndexRequest.cc +++ b/src/replica/WorkerDirectorIndexRequest.cc @@ -178,7 +178,7 @@ string WorkerDirectorIndexRequest::_query(Connection::Ptr const& conn) const { auto const database = config->databaseInfo(_request.database()); auto const table = database.findTable(_request.director_table()); - if (!table.isDirector) { + if (!table.isDirector()) { throw invalid_argument("table '" + table.name + "' is not been configured as director in database '" + database.name + "'"); } diff --git a/src/replica/testConfiguration.cc b/src/replica/testConfiguration.cc index 9a639f6208..24e8d239f9 100644 --- a/src/replica/testConfiguration.cc +++ b/src/replica/testConfiguration.cc @@ -753,8 +753,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("Table11")); BOOST_REQUIRE_NO_THROW(table = database.findTable("Table11")); BOOST_CHECK(table.isPartitioned); - BOOST_CHECK(table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "id11")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -770,8 +770,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("MetaTable11")); BOOST_REQUIRE_NO_THROW(table = database.findTable("MetaTable11")); BOOST_CHECK(!table.isPartitioned); - BOOST_CHECK(!table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(!table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -816,8 +816,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("Table21")); BOOST_REQUIRE_NO_THROW(table = database.findTable("Table21")); BOOST_CHECK(table.isPartitioned); - BOOST_CHECK(table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "id21")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -833,8 +833,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("Table22")); BOOST_REQUIRE_NO_THROW(table = database.findTable("Table22")); BOOST_CHECK(table.isPartitioned); - BOOST_CHECK(!table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(!table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("Table21", "id22")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -849,8 +849,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("MetaTable21")); BOOST_REQUIRE_NO_THROW(table = database.findTable("MetaTable21")); BOOST_CHECK(!table.isPartitioned); - BOOST_CHECK(!table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(!table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -864,8 +864,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("MetaTable22")); BOOST_REQUIRE_NO_THROW(table = database.findTable("MetaTable22")); BOOST_CHECK(!table.isPartitioned); - BOOST_CHECK(!table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(!table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -911,8 +911,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("Table31")); BOOST_REQUIRE_NO_THROW(table = database.findTable("Table31")); BOOST_CHECK(table.isPartitioned); - BOOST_CHECK(table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "id31")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -928,8 +928,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("Table32")); BOOST_REQUIRE_NO_THROW(table = database.findTable("Table32")); BOOST_CHECK(table.isPartitioned); - BOOST_CHECK(!table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(!table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("Table31", "id32")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -944,8 +944,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("Table33")); BOOST_REQUIRE_NO_THROW(table = database.findTable("Table33")); BOOST_CHECK(table.isPartitioned); - BOOST_CHECK(!table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(!table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("Table31", "id33")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -960,8 +960,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("MetaTable31")); BOOST_REQUIRE_NO_THROW(table = database.findTable("MetaTable31")); BOOST_CHECK(!table.isPartitioned); - BOOST_CHECK(!table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(!table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -975,8 +975,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("MetaTable32")); BOOST_REQUIRE_NO_THROW(table = database.findTable("MetaTable32")); BOOST_CHECK(!table.isPartitioned); - BOOST_CHECK(!table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(!table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -990,8 +990,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("MetaTable33")); BOOST_REQUIRE_NO_THROW(table = database.findTable("MetaTable33")); BOOST_CHECK(!table.isPartitioned); - BOOST_CHECK(!table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(!table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -1035,8 +1035,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("Table41")); BOOST_REQUIRE_NO_THROW(table = database.findTable("Table41")); BOOST_CHECK(table.isPartitioned); - BOOST_CHECK(table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "id41")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -1052,8 +1052,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("Table42")); BOOST_REQUIRE_NO_THROW(table = database.findTable("Table42")); BOOST_CHECK(table.isPartitioned); - BOOST_CHECK(table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "id42")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -1069,8 +1069,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("RefMatch43")); BOOST_REQUIRE_NO_THROW(table = database.findTable("RefMatch43")); BOOST_CHECK(table.isPartitioned); - BOOST_CHECK(!table.isDirector); - BOOST_CHECK(table.isRefMatch); + BOOST_CHECK(!table.isDirector()); + BOOST_CHECK(table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("Table41", "Table41_id")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("Table42", "Table42_id")); BOOST_CHECK(table.flagColName == "flag"); @@ -1087,8 +1087,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("RefMatch44")); BOOST_REQUIRE_NO_THROW(table = database.findTable("RefMatch44")); BOOST_CHECK(table.isPartitioned); - BOOST_CHECK(!table.isDirector); - BOOST_CHECK(table.isRefMatch); + BOOST_CHECK(!table.isDirector()); + BOOST_CHECK(table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("db2.Table21", "Table21_id")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("db3.Table31", "Table31_id")); BOOST_CHECK(table.flagColName == "flag"); @@ -1133,8 +1133,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("Table51")); BOOST_REQUIRE_NO_THROW(table = database.findTable("Table51")); BOOST_CHECK(table.isPartitioned); - BOOST_CHECK(table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "id51")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -1180,8 +1180,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("Table61")); BOOST_REQUIRE_NO_THROW(table = database.findTable("Table61")); BOOST_CHECK(table.isPartitioned); - BOOST_CHECK(table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "id61")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -1197,8 +1197,8 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestReadingTables) { BOOST_CHECK(database.tableExists("MetaTable61")); BOOST_REQUIRE_NO_THROW(table = database.findTable("MetaTable61")); BOOST_CHECK(!table.isPartitioned); - BOOST_CHECK(!table.isDirector); - BOOST_CHECK(!table.isRefMatch); + BOOST_CHECK(!table.isDirector()); + BOOST_CHECK(!table.isRefMatch()); BOOST_CHECK_EQUAL(table.directorTable, DirectorTableRef("", "")); BOOST_CHECK_EQUAL(table.directorTable2, DirectorTableRef("", "")); BOOST_CHECK(table.flagColName.empty()); @@ -1290,7 +1290,7 @@ BOOST_AUTO_TEST_CASE(ConfigurationTestModifyingTables) { BOOST_REQUIRE_NO_THROW(table = database.findTable(inTable.name)); BOOST_CHECK_EQUAL(table, inTable); BOOST_CHECK(!table.isPublished); - BOOST_CHECK(table.isDirector); + BOOST_CHECK(table.isDirector()); BOOST_CHECK_EQUAL(table.uniquePrimaryKey, true); BOOST_CHECK(table.createTime != 0); BOOST_CHECK(table.publishTime == 0); From e1291fbf252ab74f0465c5a15fcd08eada0fcdc0 Mon Sep 17 00:00:00 2001 From: Igor Gaponenko Date: Fri, 20 Oct 2023 18:46:58 +0000 Subject: [PATCH 2/2] Reinforced schema validation for the director tables --- src/replica/HttpIngestModule.cc | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/replica/HttpIngestModule.cc b/src/replica/HttpIngestModule.cc index f2dd6563ab..87d1c63052 100644 --- a/src/replica/HttpIngestModule.cc +++ b/src/replica/HttpIngestModule.cc @@ -33,6 +33,7 @@ // Third party headers #include "boost/filesystem.hpp" +#include "boost/algorithm/string.hpp" // Qserv headers #include "css/CssAccess.h" @@ -97,6 +98,23 @@ string jobCompletionErrorIfAny(SqlJob::Ptr const& job, string const& prefix) { return error; } +/** + * Check if the provided type is not prohibited for using in the 'director' tables. + * @param func A context for error reporting. + * @param colName The name of a column (used for error reporting). + * @param colType The column type definition to be evaluated. + * @throw lsst::qserv::replica::HttpError if the type validation failed. + */ +void validateColumnType(string const& func, string const& colName, string const& colType) { + string const colTypeUpperCase = boost::algorithm::to_upper_copy(colType); + for (char const* prohibitedType : {"BLOB", "TEXT"}) { + if (colTypeUpperCase.find(prohibitedType) == string::npos) continue; + string const msg = "the prohibited type '" + colType + "' detected in a definition of the column '" + + colName + "' of the director table"; + throw HttpError(func, msg); + } +} + } // namespace namespace lsst::qserv::replica { @@ -565,7 +583,13 @@ json HttpIngestModule::_addTable() { throw HttpError(__func__, msg); } string colType = column["type"]; - + if (table.isDirector()) { + // Schemas of the director tables require reinforced screening of the column + // types to prevent large variable size types from being used in column + // definitions. Such types will prevent Qserv from materialzing sub-chunks + // as the MEMORY tables. + ::validateColumnType(__func__, colName, colType); + } if (_partitionByColumn == colName) { string const msg = "reserved column '" + _partitionByColumn + "' is not allowed"; throw HttpError(__func__, msg);