mirror of
https://github.com/scylladb/scylladb.git
synced 2026-04-29 20:57:00 +00:00
Merge 'paxos_state: keep prepared message alive across statement execution' from Petr Gusev
In do_execute_cql_with_timeout(), when the prepared statement was not found in the cache, we called qp.prepare() and stored the returned result_message::prepared in a local variable scoped to the 'if' block. We then extracted ps_ptr (a checked_weak_ptr to the prepared statement) from the message, let the message go out of scope at the end of the 'if', and used ps_ptr after a co_await on st->execute().
Since 3ac4e258e8 ("transport/messages: hold pinned prepared entry in PREPARE result"), result_message::prepared owns a strong pinned reference to the prepared cache entry. While qp.prepare() runs it also holds its own pin on the entry, so on return the entry has at least the pin owned by the returned message. As long as that message is alive, the cache entry cannot be purged and the weak handle inside ps_ptr remains promotable.
The lifetime gap manifested only in debug builds. qp.prepare() returns a ready future on the cache-miss path, so in release builds the co_await resumes synchronously: control flows from the assignment of ps_ptr straight into st->execute() with no opportunity for any other task (in particular, prepared cache invalidation triggered by a concurrent schema change) to run in between. Debug builds, however, force a reactor preemption point on every co_await even when the awaited future is ready. With prepared_msg already destroyed at the end of the 'if' block, the only remaining handle on the cache entry was the weak ps_ptr, and the preemption gave a concurrent cache purge
- triggered, for example, by Raft schema changes received during a node restart - the chance to drop the entry. The subsequent execute() then failed when promoting the weak pointer with
checked_ptr_is_null_exception.
The exception propagated out of the Paxos prepare path as a generic std::exception with no type information in the log, surfacing on the coordinator as:
WriteFailure: Failed to prepare ballot ... Replica errors:
host_id ... -> seastar::rpc::remote_verb_error (std::exception)
Hoist the result_message::prepared into the outer scope so the pinned cache entry stays alive across co_await st->execute(...), closing the window in which a concurrent cache purge could invalidate the weak handle.
Fixes SCYLLADB-1173
backport: the patch is simple, we can backport it to all versions with "LWT over tablets" feature. Note that the problem is only in test runs in debug configuration, production is not affected.
Closes scylladb/scylladb#29675
* https://github.com/scylladb/scylladb:
table_helper: retry insert prepare on concurrent cache invalidation
paxos_state: keep prepared message alive across statement execution
This commit is contained in:
@@ -438,9 +438,10 @@ static future<cql3::untyped_result_set> do_execute_cql_with_timeout(sstring req,
|
||||
|
||||
const auto cache_key = qp.compute_id(req, "", cql3::internal_dialect());
|
||||
auto ps_ptr = qp.get_prepared(cache_key);
|
||||
shared_ptr<cql_transport::messages::result_message::prepared> prepared_msg;
|
||||
if (!ps_ptr) {
|
||||
const auto msg_ptr = co_await qp.prepare(req, qs, cql3::internal_dialect());
|
||||
ps_ptr = msg_ptr->get_prepared();
|
||||
prepared_msg = co_await qp.prepare(req, qs, cql3::internal_dialect());
|
||||
ps_ptr = prepared_msg->get_prepared();
|
||||
if (!ps_ptr) {
|
||||
on_internal_error(paxos_state::logger, "prepared statement is null");
|
||||
}
|
||||
@@ -449,8 +450,8 @@ static future<cql3::untyped_result_set> do_execute_cql_with_timeout(sstring req,
|
||||
-1, service::node_local_only::yes);
|
||||
const auto st = ps_ptr->statement;
|
||||
|
||||
const auto msg_ptr = co_await st->execute(qp, qs, qo, std::nullopt);
|
||||
co_return cql3::untyped_result_set(msg_ptr);
|
||||
const auto result_ptr = co_await st->execute(qp, qs, qo, std::nullopt);
|
||||
co_return cql3::untyped_result_set(result_ptr);
|
||||
}
|
||||
|
||||
template <typename... Args>
|
||||
|
||||
@@ -135,7 +135,23 @@ future<> table_helper::cache_table_info(cql3::query_processor& qp, service::migr
|
||||
}
|
||||
|
||||
future<> table_helper::insert(cql3::query_processor& qp, service::migration_manager& mm, service::query_state& qs, noncopyable_function<cql3::query_options ()> opt_maker) {
|
||||
co_await cache_table_info(qp, mm, qs);
|
||||
// _prepared_stmt is a checked_weak_ptr into the prepared statements
|
||||
// cache and can be invalidated by a concurrent purge (e.g. on a schema
|
||||
// change). cache_table_info() (re-)prepares and assigns _prepared_stmt,
|
||||
// but the pin protecting the entry is dropped when try_prepare()
|
||||
// returns. In release the chain of ready-future co_awaits back to here
|
||||
// resumes synchronously, but debug builds preempt on every co_await
|
||||
// even for ready futures, opening a window for a purge to drop the
|
||||
// entry and leave _prepared_stmt null. Loop until a synchronous
|
||||
// post-resume check finds _prepared_stmt valid; nothing can run between
|
||||
// that check and the dereference below. _insert_stmt is a strong
|
||||
// shared_ptr and is not affected by cache invalidation.
|
||||
while (true) {
|
||||
co_await cache_table_info(qp, mm, qs);
|
||||
if (_prepared_stmt) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
auto opts = opt_maker();
|
||||
opts.prepare(_prepared_stmt->bound_names);
|
||||
co_await _insert_stmt->execute(qp, qs, opts, std::nullopt);
|
||||
|
||||
Reference in New Issue
Block a user