Skip to content

Commit

Permalink
[#23330] docdb: fixed static columns handling for CQL operations
Browse files Browse the repository at this point in the history
Summary:
For CQL tables with primary key which have both hash and range components, `INSERT INTO INTO ... RETURNS STATUS AS ROW` might do useless extra reads. In case when then there are a lot of deleted (not yet compacted) rows with the same hash key column value, it might read all these rows during processing INSERT. In its turn this might cause growth of `docdb_obsolete_keys_found`, `docdb_obsolete_keys_found` metrics. When part of those deleted keys is already behind history cutoff limit (15 minutes by default) metric `docdb_obsolete_keys_found_past_cutoff` will also be increased and this can lead to triggering automatic full compactions for affected tablets resulting in additional system load.

This behaviour starts with the following code change (f68853b) inside `CreateProjections` function for CQL operations: https://github.com/yugabyte/yugabyte-db/blame/889f44fb8153b9535663542d5bf4b4824c9da983/src/yb/docdb/cql_operation.cc#L135. It affects `QLWriteOperation::ReadColumns` logic in the following way. The change causes
`static_projection` to always contain hash key columns and being non-empty even if we are processing `INTO INTO ... RETURNS STATUS AS ROW` without modifying static columns (and even if we don't have static columns at all). This was required due to updated read path logic that is now reading values for required columns only but not for all. But as a side effect, this leads to `QLWriteOperation::ReadColumns` calling `QLWriteOperation::InitializeKeys` with `hashed_key = !static_projection->columns.empty() == true`. Because of this `QLWriteOperation::InitializeKeys` sets `hashed_doc_key_` to non-empty value. And due to this `QLWriteOperation::ReadColumns` tries to find existing rows in DocDB querying only by hashed column value (even if primary key has range columns): https://github.com/yugabyte/yugabyte-db/blame/889f44fb8153b9535663542d5bf4b4824c9da983/src/yb/docdb/cql_operation.cc#L499.

Due to TTL, there could be a lot of obsolete rows with the same hashed column value and `DocRowwiseIterator` skips them during iteration when trying to find a live row.
This causes high growth of both `docdb_obsolete_keys_found` and `docdb_obsolete_keys_found_past_cutoff` (when huge amount of obsolete keys is already behind history cutoff).

Added unit-test for catching this bug and implemented fix for `QLWriteOperation::ReadColumns` function.
Jira: DB-12255

Test Plan: `ybd --cxx-test integration-tests_cql-test --gtest_filter CqlTest.InsertHashAndRangePkWithReturnsStatusAsRow -n 50 -- -p 1` for asan/tsan/debug/release.

Reviewers: sergei, rthallam, arybochkin

Reviewed By: sergei, arybochkin

Subscribers: yql, ybase

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D36921
  • Loading branch information
ttyusupov committed Aug 1, 2024
1 parent 291dd40 commit 75cb273
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/yb/docdb/cql_operation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ Status QLWriteOperation::ReadColumns(const DocOperationApplyData& data,
// Generate hashed / primary key depending on if static / non-static columns are referenced in
// the if-condition.
RETURN_NOT_OK(InitializeKeys(
!static_projection->columns.empty(), !non_static_projection->columns.empty()));
!request_.column_refs().static_ids().empty(), !non_static_projection->columns.empty()));

// Scan docdb for the static and non-static columns of the row using the hashed / primary key.
if (hashed_doc_key_) {
Expand Down
48 changes: 48 additions & 0 deletions src/yb/integration-tests/cql-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1540,4 +1540,52 @@ TEST_F(CqlTest, RetainSchemaPacking) {
LOG(INFO) << "Content: " << content;
}

TEST_F(CqlTest, InsertHashAndRangePkWithReturnsStatusAsRow) {
constexpr auto TTL_SECONDS = 2;
constexpr auto INSERT_PHASE_SECONDS = TTL_SECONDS + 3;

constexpr auto HASH_COLUMN_VALUE = 12345678;

auto session = ASSERT_RESULT(EstablishSession(driver_.get()));

ASSERT_OK(session.ExecuteQuery(
"CREATE TABLE test (h int, r int, v int, PRIMARY KEY (h, r)) "
"WITH CLUSTERING ORDER BY (r ASC) AND transactions = {'enabled': 'false'}"));

auto insert_prepared = ASSERT_RESULT(session.Prepare(Format(
"INSERT INTO test (h, r, v) VALUES (?,?,?) USING TTL $0 RETURNS STATUS AS ROW",
TTL_SECONDS)));

int row_count = 0;

auto deadline = CoarseMonoClock::now() + INSERT_PHASE_SECONDS * 1s;
while (CoarseMonoClock::now() < deadline) {
auto stmt = insert_prepared.Bind();
stmt.Bind(0, HASH_COLUMN_VALUE);
stmt.Bind(1, row_count);
stmt.Bind(2, row_count);
ASSERT_OK(session.Execute(stmt));
++row_count;
YB_LOG_EVERY_N_SECS(INFO, 5) << row_count << " rows inserted";
}

ASSERT_GT(row_count, 0) << "We expect some rows to be inserted";

// Make sure `RETURNS STATUS AS ROW` doesn't iterate over rows (both obsolete and live) related to
// the same hash column but different range columns.
for (auto peer : ListTabletPeers(cluster_.get(), ListPeersFilter::kAll)) {
auto tablet = peer->shared_tablet();
if (tablet->table_type() != TableType::YQL_TABLE_TYPE) {
break;
}
auto* metrics = tablet->metrics();
for (auto counter :
{tablet::TabletCounters::kDocDBKeysFound, tablet::TabletCounters::kDocDBObsoleteKeysFound,
tablet::TabletCounters::kDocDBObsoleteKeysFoundPastCutoff}) {
ASSERT_EQ(metrics->Get(counter), 0)
<< "Expected " << counter << " to be zero for tablet peer " << peer->LogPrefix();
}
}
}

} // namespace yb

0 comments on commit 75cb273

Please sign in to comment.