Creating a table which looks like a secondary index breaks the secondary idnex creation mechanism

[psarna]

When testing an unrelated change, I came across a way to ;eave the secondary index creation mechanism in Scylla in a broken state.

Reproducer:

CREATE KEYSPACE ks WITH replication = {'class': 'SimpleStrategy', 'replication_factor': 1};
use ks;
CREATE TABLE t (id int primary key, v int);
CREATE TABLE t_v_idx_index (id int primary key);
CREATE INDEX ON t(v);
DROP TABLE t_v_idx_index;

The snippet exploits the fact that the internal table created for the secondary index is named after a pattern - in this case, an index on t(v) is going to be named t_v_idx, and its table is going to be t_v_idx_index.

Since we happened to create a regular table with an identical name, creating an index fails. The first minor bug is that the error message says ServerError: Column family t3_v_idx_index exists, which is not strictly true - such index does not exist, there just happens to be a table named identically. It gets worse:

1. Trying to remove the table results in a cryptic error which makes little sense: ServerError: Can't find a column family with UUID 1027b711-b18f-11eb-bcd4-ab78603bce8b. The table was just created, so why can't we drop it?
2. Dropping an index also doesn't work (that's expected), but the index is listed in the local schema tables:

 keyspace_name | table_name | index_name | kind       | options
---------------+------------+------------+------------+-----------------
            ks |          t |    t_v_idx | COMPOSITES | {'target': 'v'}

3. What's even more weird, our t_v_idx_index table seems to not exist in the schema tables, so its removal was actually successful, even though it returned an error:

cassandra@cqlsh:ks> select table_name from system_schema. tables where keyspace_name = 'ks';

 table_name
------------
          t

This bug is not particularly dangerous, since it requires creating a maliciously named table, but it's nonetheless a bug. Also, a trivial workaround is to created a named index - CREATE INDEX hello ON t(v) works just fine.

[psarna]

edit: It's even worse when the fake table contains column names not found in the base table. It returns an ugly internal error with a backtrace. A new reproducer:

CREATE KEYSPACE ks WITH replication = {'class': 'SimpleStrategy', 'replication_factor': 1};
use ks;
CREATE TABLE t (id int primary key, v int);
CREATE TABLE t_v_idx_index (this_column_name_does_not_exist_in_t int primary key);
CREATE INDEX ON t(v);
DROP TABLE t_v_idx_index;

And here's the ugly result:

cassandra@cqlsh:ks> DROP TABLE t_v_idx_index;
ServerError: base_schema(): operation unsupported when initialized only for view reads. Missing column in the base table: this_column_name_does_not_exist_in_t Backtrace: 0x254d98e 0x254de30 0x254e1d8 0x1b33e72 0x1b33d16 0x2189e70 0x2189d14 0x2189b82 0x19ed931 0x12d30ce 0x18ff943 0x18f8b3c 0x18f7db3 0x24382d1
   --------
   seastar::continuation<seastar::internal::promise_base_with_type<void>, seastar::async<db::schema_tables::do_merge_schema(seastar::sharded<service::storage_proxy>&, std::vector<mutation, std::allocator<mutation> >, bool)::$_16>(seastar::thread_attributes, db::schema_tables::do_merge_schema(seastar::sharded<service::storage_proxy>&, std::vector<mutation, std::allocator<mutation> >, bool)::$_16&&)::{lambda()#2}, seastar::future<void>::then_impl_nrvo<{lambda()#2}, seastar::future>(db::schema_tables::do_merge_schema(seastar::sharded<service::storage_proxy>&, std::vector<mutation, std::allocator<mutation> >, bool)::$_16&&)::{lambda(seastar::internal::promise_base_with_type<void>&&, {lambda()#2}&, seastar::future_state<seastar::internal::monostate>&&)#1}, void>
   --------
   seastar::continuation<seastar::internal::promise_base_with_type<void>, seastar::future<void>::finally_body<seastar::async<db::schema_tables::do_merge_schema(seastar::sharded<service::storage_proxy>&, std::vector<mutation, std::allocator<mutation> >, bool)::$_16>(seastar::thread_attributes, db::schema_tables::do_merge_schema(seastar::sharded<service::storage_proxy>&, std::vector<mutation, std::allocator<mutation> >, bool)::$_16&&)::{lambda()#3}, false>, seastar::future<void>::then_wrapped_nrvo<seastar::future<void>, seastar::async<db::schema_tables::do_merge_schema(seastar::sharded<service::storage_proxy>&, std::vector<mutation, std::allocator<mutation> >, bool)::$_16>(seastar::thread_attributes, db::schema_tables::do_merge_schema(seastar::sharded<service::storage_proxy>&, std::vector<mutation, std::allocator<mutation> >, bool)::$_16&&)::{lambda()#3}>(seastar::async<db::schema_tables::do_merge_schema(seastar::sharded<service::storage_proxy>&, std::vector<mutation, std::allocator<mutation> >, bool)::$_16>(seastar::thread_attributes, db::schema_tables::do_merge_schema(seastar::sharded<service::storage_proxy>&, std::vector<mutation, std::allocator<mutation> >, bool)::$_16&&)::{lambda()#3}&&)::{lambda(seastar::internal::promise_base_with_type<void>&&, seastar::async<db::schema_tables::do_merge_schema(seastar::sharded<service::storage_proxy>&, std::vector<mutation, std::allocator<mutation> >, bool)::$_16>(seastar::thread_attributes, auto:1&&)::{lambda()#3}&, seastar::future_state<seastar::internal::monostate>&&)#1}, void>
   --------
   seastar::continuation<seastar::internal::promise_base_with_type<void>, db::schema_tables::merge_schema(seastar::sharded<service::storage_proxy>&, gms::feature_service&, std::vector<mutation, std::allocator<mutation> >)::$_11::operator()()::{lambda()#1}, seastar::future<void>::then_impl_nrvo<{lambda()#1}, seastar::future>({lambda()#1}&&)::{lambda(seastar::internal::promise_base_with_type<void>&&, {lambda()#1}&, seastar::future_state<seastar::internal::monostate>&&)#1}, void>
   --------
   seastar::continuation<seastar::internal::promise_base_with_type<void>, seastar::future<void>::finally_body<db::schema_tables::merge_schema(seastar::sharded<service::storage_proxy>&, gms::feature_service&, std::vector<mutation, std::allocator<mutation> >)::$_12, true>, seastar::future<void>::then_wrapped_nrvo<seastar::future<void>, seastar::future<void>::finally_body<db::schema_tables::merge_schema(seastar::sharded<service::storage_proxy>&, gms::feature_service&, std::vector<mutation, std::allocator<mutation> >)::$_12, true> >(seastar::future<void>::finally_body<db::schema_tables::merge_schema(seastar::sharded<service::storage_proxy>&, gms::feature_service&, std::vector<mutation, std::allocator<mutation> >)::$_12, true>&&)::{lambda(seastar::internal::promise_base_with_type<void>&&, seastar::future<void>::finally_body<db::schema_tables::merge_schema(seastar::sharded<service::storage_proxy>&, gms::feature_service&, std::vector<mutation, std::allocator<mutation> >)::$_12, true>&, seastar::future_state<seastar::internal::monostate>&&)#1}, void>
   --------
   seastar::continuation<seastar::internal::promise_base_with_type<seastar::shared_ptr<cql_transport::event::schema_change> >, cql3::statements::drop_table_statement::announce_migration(cql3::query_processor&) const::$_1, seastar::future<void>::then_wrapped_nrvo<seastar::shared_ptr<cql_transport::event::schema_change>, cql3::statements::drop_table_statement::announce_migration(cql3::query_processor&) const::$_1>(cql3::statements::drop_table_statement::announce_migration(cql3::query_processor&) const::$_1&&)::{lambda(seastar::internal::promise_base_with_type<seastar::shared_ptr<cql_transport::event::schema_change> >&&, cql3::statements::drop_table_statement::announce_migration(cql3::query_processor&) const::$_1&, seastar::future_state<seastar::internal::monostate>&&)#1}, void>
   --------
   seastar::continuation<seastar::internal::promise_base_with_type<seastar::shared_ptr<cql_transport::messages::result_message> >, cql3::statements::schema_altering_statement::execute0(cql3::query_processor&, service::query_state&, cql3::query_options const&) const::$_0, seastar::future<seastar::shared_ptr<cql_transport::event::schema_change> >::then_impl_nrvo<cql3::statements::schema_altering_statement::execute0(cql3::query_processor&, service::query_state&, cql3::query_options const&) const::$_0, seastar::future<seastar::shared_ptr<cql_transport::messages::result_message> > >(cql3::statements::schema_altering_statement::execute0(cql3::query_processor&, service::query_state&, cql3::query_options const&) const::$_0&&)::{lambda(seastar::internal::promise_base_with_type<seastar::shared_ptr<cql_transport::messages::result_message> >&&, cql3::statements::schema_altering_statement::execute0(cql3::query_processor&, service::query_state&, cql3::query_options const&) const::$_0&, seastar::future_state<seastar::shared_ptr<cql_transport::event::schema_change> >&&)#1}, seastar::shared_ptr<cql_transport::event::schema_change> >
   --------
   seastar::continuation<seastar::internal::promise_base_with_type<seastar::shared_ptr<cql_transport::messages::result_message> >, cql3::statements::schema_altering_statement::execute(cql3::query_processor&, service::query_state&, cql3::query_options const&) const::$_1, seastar::future<seastar::shared_ptr<cql_transport::messages::result_message> >::then_impl_nrvo<cql3::statements::schema_altering_statement::execute(cql3::query_processor&, service::query_state&, cql3::query_options const&) const::$_1, seastar::future<seastar::shared_ptr<cql_transport::messages::result_message> > >(cql3::statements::schema_altering_statement::execute(cql3::query_processor&, service::query_state&, cql3::query_options const&) const::$_1&&)::{lambda(seastar::internal::promise_base_with_type<seastar::shared_ptr<cql_transport::messages::result_message> >&&, cql3::statements::schema_altering_statement::execute(cql3::query_processor&, service::query_state&, cql3::query_options const&) const::$_1&, seastar::future_state<seastar::shared_ptr<cql_transport::messages::result_message> >&&)#1}, seastar::shared_ptr<cql_transport::messages::result_message> >
   --------
   seastar::continuation<seastar::internal::promise_base_with_type<seastar::shared_ptr<cql_transport::messages::result_message> >, cql3::query_processor::process_authorized_statement(seastar::shared_ptr<cql3::cql_statement>, service::query_state&, cql3::query_options const&)::$_4, seastar::future<seastar::shared_ptr<cql_transport::messages::result_message> >::then_impl_nrvo<cql3::query_processor::process_authorized_statement(seastar::shared_ptr<cql3::cql_statement>, service::query_state&, cql3::query_options const&)::$_4, seastar::future<seastar::shared_ptr<cql_transport::messages::result_message> > >(cql3::query_processor::process_authorized_statement(seastar::shared_ptr<cql3::cql_statement>, service::query_state&, cql3::query_options const&)::$_4&&)::{lambda(seastar::internal::promise_base_with_type<seastar::shared_ptr<cql_transport::messages::result_message> >&&, cql3::query_processor::process_authorized_statement(seastar::shared_ptr<cql3::cql_statement>, service::query_state&, cql3::query_options const&)::$_4&, seastar::future_state<seastar::shared_ptr<cql_transport::messages::result_message> >&&)#1}, seastar::shared_ptr<cql_transport::messages::result_message> >
   --------
   seastar::continuation<seastar::internal::promise_base_with_type<std::variant<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > >, unsigned int> >, cql_transport::process_query_internal(service::client_state&, seastar::sharded<cql3::query_processor>&, cql_transport::request_reader, unsigned short, unsigned char, cql_serialization_format, service_permit, tracing::trace_state_ptr, bool)::$_12, seastar::future<seastar::shared_ptr<cql_transport::messages::result_message> >::then_impl_nrvo<cql_transport::process_query_internal(service::client_state&, seastar::sharded<cql3::query_processor>&, cql_transport::request_reader, unsigned short, unsigned char, cql_serialization_format, service_permit, tracing::trace_state_ptr, bool)::$_12, seastar::future<std::variant<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > >, unsigned int> > >(cql_transport::process_query_internal(service::client_state&, seastar::sharded<cql3::query_processor>&, cql_transport::request_reader, unsigned short, unsigned char, cql_serialization_format, service_permit, tracing::trace_state_ptr, bool)::$_12&&)::{lambda(seastar::internal::promise_base_with_type<std::variant<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > >, unsigned int> >&&, cql_transport::process_query_internal(service::client_state&, seastar::sharded<cql3::query_processor>&, cql_transport::request_reader, unsigned short, unsigned char, cql_serialization_format, service_permit, tracing::trace_state_ptr, bool)::$_12&, seastar::future_state<seastar::shared_ptr<cql_transport::messages::result_message> >&&)#1}, seastar::shared_ptr<cql_transport::messages::result_message> >
   --------
   seastar::continuation<seastar::internal::promise_base_with_type<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > > >, cql_transport::cql_server::connection::process<seastar::future<std::variant<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > >, unsigned int> > (*)(service::client_state&, seastar::sharded<cql3::query_processor>&, cql_transport::request_reader, unsigned short, unsigned char, cql_serialization_format, service_permit, tracing::trace_state_ptr, bool)>(unsigned short, cql_transport::request_reader, service::client_state&, service_permit, tracing::trace_state_ptr, seastar::future<std::variant<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > >, unsigned int> > (*)(service::client_state&, seastar::sharded<cql3::query_processor>&, cql_transport::request_reader, unsigned short, unsigned char, cql_serialization_format, service_permit, tracing::trace_state_ptr, bool))::{lambda(std::variant<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > >, unsigned int>)#1}, seastar::future<std::variant<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > >, unsigned int> >::then_impl_nrvo<{lambda(std::variant<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > >, unsigned int>)#1}, seastar::future<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > > > >({lambda(std::variant<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > >, unsigned int>)#1}&&)::{lambda(seastar::internal::promise_base_with_type<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > > >&&, {lambda(std::variant<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > >, unsigned int>)#1}&, seastar::future_state<std::variant<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > >, unsigned int> >&&)#1}, std::variant<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > >, unsigned int> >
   --------
   seastar::continuation<seastar::internal::promise_base_with_type<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > > >, cql_transport::cql_server::connection::process_request_one(fragmented_temporary_buffer::istream, unsigned char, unsigned short, service::client_state&, cql_transport::cql_server::connection::tracing_request_type, service_permit)::$_3, seastar::future<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > > >::then_wrapped_nrvo<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > >, cql_transport::cql_server::connection::process_request_one(fragmented_temporary_buffer::istream, unsigned char, unsigned short, service::client_state&, cql_transport::cql_server::connection::tracing_request_type, service_permit)::$_3>(cql_transport::cql_server::connection::process_request_one(fragmented_temporary_buffer::istream, unsigned char, unsigned short, service::client_state&, cql_transport::cql_server::connection::tracing_request_type, service_permit)::$_3&&)::{lambda(seastar::internal::promise_base_with_type<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > > >&&, cql_transport::cql_server::connection::process_request_one(fragmented_temporary_buffer::istream, unsigned char, unsigned short, service::client_state&, cql_transport::cql_server::connection::tracing_request_type, service_permit)::$_3&, seastar::future_state<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > > >&&)#1}, seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<cql_transport::response> > > >
   --------
   seastar::continuation<seastar::internal::promise_base_with_type<void>, cql_transport::cql_server::connection::process_request()::$_4::operator()(seastar::future<std::optional<cql_transport::cql_binary_frame_v3> >&&) const::{lambda(seastar::semaphore_units<seastar::semaphore_default_exception_factory, std::chrono::_V2::steady_clock>)#1}::operator()(seastar::semaphore_units<seastar::semaphore_default_exception_factory, std::chrono::_V2::steady_clock>) const::{lambda(fragmented_temporary_buffer)#1}::operator()({lambda(seastar::semaphore_units<seastar::semaphore_default_exception_factory, std::chrono::_V2::steady_clock>)#1})::{lambda(seastar::future<seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<seastar::foreign_ptr> > > >)#1}, std::unique_ptr<cql_transport::response, std::default_delete<seastar::foreign_ptr> >::then_wrapped_nrvo<void, seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<seastar::foreign_ptr> > > >(seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<seastar::foreign_ptr> > >&&)::{lambda(seastar::internal::promise_base_with_type<void>&&, seastar::foreign_ptr<std::unique_ptr<cql_transport::response, std::default_delete<seastar::foreign_ptr> > >&, seastar::future_state<std::default_delete<seastar::foreign_ptr> >&&)#1}, std::default_delete<seastar::foreign_ptr> >

[nyh]

I think we have two options now:

1. We can forbid creating any table called "something_index" - because this is the name we will use for a table named "something" (note that this problem is not specific to the default choice of names like in your examples). This is the simplest solution, but I don't like it.
2. We can detect conflicts before they start, and return a clear error. For example, if you already created a table "hello_index" and now trying to create an index called "hello" you should get an error telling you exactly what happened - that you can't call an index "hello" because the name "hello_index" is already taken. Note that in this option, we need to make sure we correctly detect indexes - a table or even a materialized view doesn't become an index just because it's called "something_index". We need to have a separate flag for this (and I think we already do!)

As I said I prefer option 2. Or is there any other option?

[psarna]

Yes, option 2 is the way to go in my opinion. We already have this check in the code (because once the index tries to create its internal table, it fails), but the problem is that the operation is definitely not rolled back properly - the index creation process is left in a half-baked, undefined state. This issue can probably be fixed simply by adding an additional check if a table with conflicting name exists. Then we'll probably still be susceptible for rare races in case we check that there's no conflicting name, then yield, and then somebody else creates a table with the name we just checked, but that's just an example of a whole class of problems we currently have with concurrent schema changes. All of these will be void once we switch to Raft, so perhaps it's not worth going too far with the investigation.

[nyh]

It's important to backport this fix because we had a customer that without it experienced not only an annoying message but an assertion failure on reboot after we are left with an inconsistent case.
Starting backports now.

[nyh]

Backported to next-4.5 (89fbcf9)
I'm having build problems for earlier releases, I'll continue to backport to 4.4 and 4.3 soon.

[nyh]

Backported also to next-4.4 (cb3225f).

Sadly, the backport to next-4.3 has a big problem - the patch applies, but the resulting code doesn't pass the test. It gives a strange "base_schema(): operation unsupported when initialized only for view reads" error. @eliransin any thoughts on this? It seems an area you fixed in the past, are there any patches in this area which we may not have backported to 4.3?

[nyh]

I did the backport to next-4.3 again, and now it works. Maybe I did something wrong last time. The backport is: 2e7f618