Commit Graph

3354 Commits

Author SHA1 Message Date
Eliran Sinvani
c5956957f3 use_statement: Covert an exception to a future exception
The use statement execution code can throw if the keyspace is
doesn't exist, this can be a problem for code that will use
execute in a fiber since the exception will break the fiber even
if `then_wrapped` is used.

Fixes #14449

Signed-off-by: Eliran Sinvani <eliransin@scylladb.com>

Closes scylladb/scylladb#14394
2023-11-09 10:43:11 +01:00
Kefu Chai
cf70970226 build: cmake: use $<CONFIG:cfgs> when appropriate
since CMake 3.19, we are able to use $<CONFIG:cfgs> instead of
the more cubersume $<IN_LIST:$<CONFIG>,foo;bar> expression for
checking if a config is in a list of configurations.
and since the minimal required CMake of scylla is 3.27, so let's
use $<CONFIG:cfgs> when possible.

see also https://cmake.org/cmake/help/git-stage/manual/cmake-generator-expressions.7.html#configuration-expressions

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>

Closes scylladb/scylladb#15989
2023-11-08 08:50:44 +02:00
Kefu Chai
882e7eca25 build: cmake: set compile options with generator expression
instead of using a single compile option for all modes, use per-mode
compile options. this change keeps us away from using `CMAKE_BUILD_TYPE`
directly, and prepares us for the multi-config generator support.

because we only apply these settings in the configurations where
sanitizers are used, there is no need to check if these option can be
accepted by the compiler. if this turns out to be a problem, we can
always add the check back on a per-mode basis.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
2023-11-07 10:35:20 +08:00
Kefu Chai
23bb644314 build: cmake: drop build_mode
there is no benefit having this variable. and it introduces
another layer of indirection. so drop it.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
2023-11-07 10:10:59 +08:00
Benny Halevy
a1acf6854b everywhere: reduce dependencies on i_partitioner.hh
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
2023-11-05 20:47:44 +02:00
Piotr Smaroń
8c464b2ddb guardrails: restrict replication strategy (RS)
Replacing `restrict_replication_simplestrategy` config option with
2 config options: `replication_strategy_{warn,fail}_list`, which
allow us to impose soft limits (issue a warning) and hard limits (not
execute CQL) on replication strategy when creating/altering a keyspace.
The reason to rather replace than extend `restrict_replication_simplestrategy` config
option is that it was not used and we wanted to generalize it.
Only soft guardrail is enabled by default and it is set to SimpleStrategy,
which means that we'll generate a CQL warning whenever replication strategy
is set to SimpleStrategy. For new cloud deployments we'll move
SimpleStrategy from warn to the fail list.
Guardrails violations will be tracked by metrics.

Resolves #5224
Refs #8892 (the replication strategy part, not the RF part)

Closes scylladb/scylladb#15399
2023-10-31 18:34:41 +03:00
Avi Kivity
ef7db6df99 Merge 'schema_tables: turn view schema fixing code into a sanity check' from Kamil Braun
The purpose of `maybe_fix_legacy_secondary_index_mv_schema` was to deal
with legacy materialized view schemas used for secondary indexes,
schemas which were created before the notion of "computed columns" was
introduced. Back then, secondary index schemas would use a regular
"token" column. Later it became a computed column and old schemas would
be migrated during rolling upgrade.

The migration code was introduced in 2019
(db8d4a0cc6) and then fixed in 2020
(d473bc9b06).

The fix was present in Enterprise 2022.1 and in OSS 4.5. So, assuming
that users don't try crazy things like upgrading from 2021.X to 2023.X
(which we do not support), all clusters will have already executed the
migration code once they upgrade to 2023.X, meaning we can get rid of
it.

The main motivation of this PR is to get rid of the
`db::schema_tables::merge_schema` call in `parse_schema_tables`. In Raft
mode this was the only call to `merge_schema` outside "group 0 code" and
in fact it is unsafe -- it uses locally generated mutations with locally
generated timestamp (`api::new_timestamp()`), so if we actually did it,
we would permanently diverge the group 0 state machine across nodes
(the schema pulling code is disabled in Raft mode). Fortunately, this
should be dead code by now, as explained in the previous paragraph.

The migration code is now turned into a sanity check, if the users
try something crazy, they will get an error instead of silent data
corruption.

Closes scylladb/scylladb#15695

* github.com:scylladb/scylladb:
  view: remove unused `_backing_secondary_index`
  schema_tables: turn view schema fixing code into a sanity check
  schema_tables: make comment more precise
  feature_service: make COMPUTED_COLUMNS feature unconditionally true
2023-10-31 13:23:19 +02:00
Kefu Chai
d01b9f95a0 build: cmake: disable sanitize-address-use-after-scope only when needed
we enable sanitizer only in Debug and Sanitize build modes, if we pass
`-fno-sanitize-address-use-after-scope` to compiler when the sanitizer
is not enabled when compiling, Clang complains like:

```
clang-16: error: argument unused during compilation: '-fno-sanitize-address-use-after-scope' [-Werror,-Wunused-command-line-argument]
```

this breaks the build on the build modes where sanitizers are not
enabled.

so, in this change, we only disable the sanitize-address-use-after-scope
sanitizer if the sanitizers are enabled.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>

Closes scylladb/scylladb#15868
2023-10-30 20:14:12 +02:00
Kefu Chai
d43afd576e cql3/restrictions/statement_restrictions: s/allow filtering/ALLOW FILTERING/
use the captalized "ALLOW FILTERING" in the error message, because the
error message is a part of the user interface, it would be better to
keep it aligned with our document, where "ALLOW FILTERING" is used.

so, in this change, the lower-cased "allow filtering" error message is
changed to "ALLOW FILTERING", and the tests are updated accordingly.

see also a0ffbf3291

Refs #14321
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>

Closes scylladb/scylladb#15718
2023-10-26 10:00:37 +03:00
Avi Kivity
782c6a208a Merge 'cql3: mutation_fragments_select_statement: keep erm alive for duration of the query' from Botond Dénes
Said statement keeps a reference to erm indirectly, via a topology node pointer, but doesn't keep erm alive. This can result in use-after-free. Furthermore, it allows for vnodes being pulled from under the query's feet, as it is running.
To prevent this, keep the erm alive for the duration of the query.
Also, use `host_id` instead of `node`, the node pointer is not needed really, as the statement only uses the host id from it.

Fixes: #15802

Closes scylladb/scylladb#15808

* github.com:scylladb/scylladb:
  cql3: mutation_fragments_select_statement: use host_id instead of node
  cql3: mutation_fragments_select_statement: pin erm reference
2023-10-25 15:03:07 +03:00
Kamil Braun
3976808b12 schema_tables: turn view schema fixing code into a sanity check
The purpose of `maybe_fix_legacy_secondary_index_mv_schema` was to deal
with legacy materialized view schemas used for secondary indexes,
schemas which were created before the notion of "computed columns" was
introduced. Back then, secondary index schemas would use a regular
"token" column. Later it became a computed column and old schemas would
be migrated during rolling upgrade.

The migration code was introduced in 2019
(db8d4a0cc6) and then fixed in 2020
(d473bc9b06).

The fix was present in Enterprise 2022.1 and in OSS 4.5. So, assuming
that users don't try crazy things like upgrading from 2021.X to 2023.X
(which we do not support), all clusters will have already executed the
migration code once they upgrade to 2023.X, meaning we can get rid of
it.

The main motivation of this patch is to get rid of the
`db::schema_tables::merge_schema` call in `parse_schema_tables`. In Raft
mode this was the only call to `merge_schema` outside "group 0 code" and
in fact it is unsafe -- it uses locally generated mutations with locally
generated timestamp (`api::new_timestamp()`), so if we actually did it,
we would permanently diverge the group 0 state machine across nodes
(the schema pulling code is disabled in Raft mode). Fortunately, this
should be dead code by now, as explained in the previous paragraph.

The migration code is now turned into a sanity check, if the users
try something crazy, they will get an error instead of silent data
corruption.
2023-10-24 13:33:35 +02:00
Botond Dénes
23898581d5 cql3: mutation_fragments_select_statement: use host_id instead of node
The statement only uses the node to get its host_id later. Simpler to
obtain and store only the host_id int he first place.
2023-10-24 03:12:58 -04:00
Botond Dénes
3cb1669340 cql3: mutation_fragments_select_statement: pin erm reference
This query bypasses the usual read-path in storage-proxy and therefore
also misses the erm pinning done by storage-proxy. To avoid a vnode
being pulled from under its feet, do the erm pinning in the statement
itself.
2023-10-24 03:12:36 -04:00
Kefu Chai
9347b61d3b build: cmake: avoid using large amount stack of when compiling parser
this mirrors what we have in `configure.py`, to build the CqlParser with -O1
and disable sanitize-address-use-after-scope when compiling CqlParser.cc
in order to prevent the compiler from emitting code which uses large amount of stack
at the runtime.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
2023-10-24 12:40:20 +08:00
Kefu Chai
3da02e1bf4 build: cmake: s/COMPILE_FLAGS/COMPILE_OPTIONS/
according to
https://cmake.org/cmake/help/latest/prop_sf/COMPILE_FLAGS.html,
COMPILE_FLAGS has been superseded by COMPILE_OPTIONS. so let's
replace the former with the latter.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
2023-10-24 12:40:20 +08:00
Jan Ciolek
c256cca6f1 cql3/expr: add more comments in expression.hh
`expression` is a std::variant with 16 different variants
that represent different types of AST nodes.

Let's add documentation that explains what each of these
16 types represents. For people who are not familiar with expression
code it might not be clear what each of them does, so let's add
clear descriptions for all of them.

Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>

Closes scylladb/scylladb#15767
2023-10-19 10:56:38 +03:00
Michael Huang
75109e9519 cql3: Fix invalid JSON parsing for JSON objects with ASCII keys
For JSON objects represented as map<ascii, int>, don't treat ASCII keys
as a nested JSON string. We were doing that prior to the patch, which
led to parsing errors.

Included the error offset where JSON parsing failed for
rjson::parse related functions to help identify parsing errors
better.

Fixes: #7949

Signed-off-by: Michael Huang <michaelhly@gmail.com>

Closes scylladb/scylladb#15499
2023-10-05 22:26:08 +03:00
Avi Kivity
301b0a989a Merge ' cql3/prepare_context: fix generating pk_indexes for duplicate named bind variables' from Jan Ciołek
When presented with queries that use the same named bind variables twice, like this one:
```cql
SELECT p FROM table WHERE p = :x AND c = :x
```

Scylla generated empty `partition_key_bind_indexes` (`pk_indexes`).
`pk_indexes` tell the driver which bind variables it should use to calculate the partition token for a query. Without it, the driver is unable to determine the token and it will send the query to a random node.

Scylla should generate pk_indexes which tell the driver that it can use bind variable with `bind_index = 0` to calculate the partition token for this query.

The problem was that `_target_columns` keep only a single target_column for each bind variable.
In the example above `:x` is compared with both `p` and `c`, but `_target_columns` would contain only one of them, and Scylla wasn't able to tell that this bind variable is compared with a partition key column.

To fix it, let's replace `_target_columns` with `_targets`. `_targets` keeps all comparisons
between bind variables and other expressions, so none of them will be forgotten/overwritten.

A `cql-pytest` reproducer is added.

I also added some comments in `prepare_context.hh/cc` to make it easier to read.

Fixes: https://github.com/scylladb/scylladb/issues/15374

Closes scylladb/scylladb#15526

* github.com:scylladb/scylladb:
  cql-pytest/test-prepare: remove xfail marker from *pk_indexes_duplicate_named_variables
  cql3/prepare_context: fix generating pk_indexes for duplicate named bind variables
  cql3: improve readability of prepare_context
  cql-pytest: test generation of pk indexes during PREPARE
2023-09-26 19:47:04 +03:00
Alexander Turetskiy
024ba84637 cql3: SELECT CAST column names should match Cassandra's
When doing a SELECT CAST(b AS int), Cassandra returns a column named
cast(b as int). Currently, Scylla uses a different name -
system.castasint(b). For Cassandra compatibility, we should switch to
the same name.

fixes #14508

Closes scylladb/scylladb#14800
2023-09-26 17:26:14 +03:00
Jan Ciolek
e5f0468761 cql/prepare_expr: fix wrong receiver in field_selection_test_assignment
When preparing a `field_selection`, we need to prepare the UDT value,
and then verify that it has this field.

`field_selection_test_assignment` prepares the UDT value using the same
receiver as the whole `field_selection`. This is wrong, this receiver
has the type of the field, and not the UDT.

It's impossible to create a receiver for the UDT. Many different UDTs
can produce an `int` value when the field `a` is selected.
Therefore the receiver should be `nullptr`.

No unit test is added, as this bug doesn't currently cause any issues.
Preparing a column value doesn't do any type checks, so nothing fails.
Still it's good to fix it, just to be correct.

Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>

Closes scylladb/scylladb#14788
2023-09-26 11:15:00 +03:00
Jan Ciolek
3cff10f756 cql3/prepare_context: fix generating pk_indexes for duplicate named bind variables
When presented with queries that use the same named bind variables twice,
like this one:
```cql
SELECT p FROM table WHERE p = :x AND c = :x
```

Scylla generated empty partition_key_bind_indexes (pk_indexes).
pk_indexes tell the driver which bind variables it should use to calculate the partition
token for a query. Without it, the driver is unable to determine the token and it will
send the query to a random node.

Scylla should generate pk_indexes which tell the driver that it can use bind variable
with bind_index = 0 to calculate the partition token for a query.

The problem was that _target_columns keep only a single target_column for each bind variable.
In the example above :x is compared with both p and c, but _target_columns would contain
only one of them, and Scylla wasn't able to tell that this bind variable is compared with
a partition key column.

To fix it, let's replace _target_columns with _targets. _targets keeps all comparisons
between bind variables and other expressions, so none of them will be forgotten/overwritten.

Fixes: #15374

Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
2023-09-25 17:18:53 +02:00
Jan Ciolek
a993ae31f8 cql3: improve readability of prepare_context
This commits adds a few comments and changes a few variable names
so that it's easier to figure out what the code does.

When I first started looking at this part of the code it wasn't
obvious what's going on - what are _specs, how are they different
from _target_columns? What happens when a variable doesn't have a name?

I hope that this change will make it easier to understand for future readers.

Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
2023-09-25 17:18:53 +02:00
Nadav Har'El
4e1e7568d8 Merge 'cql3:statements:describe_statement: include UDT/UDF/UDA in generic describe' from Michał Jadwiszczak
So far generic describe (`DESC <name>`) followed Cassandra implementation and it only described keyspace/table/view/index.

This commit adds UDT/UDF/UDA to generic describe.

Fixes: #14170

Closes scylladb/scylladb#14334

* github.com:scylladb/scylladb:
  docs:cql: add information  about generic describe
  cql-pytest:test_describe: add test for generic UDT/UDF/UDA desc
  cql3:statements:describe_statement: include UDT/UDF/UDA in generic describe
2023-09-24 13:03:04 +03:00
Botond Dénes
91a8100b3f Merge 'Validate compaction strategy options in prepare' from Aleksandra Martyniuk
Table properties validation is performed on statement execution.
Thus, when one attempts to create a table with invalid options,
an incorrect command gets committed in Raft. But then its
application fails, leading to a raft machine being stopped.

Check table properties when create and alter statements are prepared.

Fixes: #14710.

Closes scylladb/scylladb#15091

* github.com:scylladb/scylladb:
  cql3: statements: delete execute override
  cql3: statements: call check_restricted_table_properties in prepare
  cql3: statements: pass data_dictionary::database to check_restricted_table_properties
2023-09-22 09:49:19 +03:00
Michael Huang
a684e51e4d cql3: fix bad optional access when executing fromJson function
Fix fromJson(null) to return null, not a error as it did before this patch.
We use "null" as the default value when unwrapping optionals
to avoid bad optional access errors.

Fixes: scylladb#7912

Signed-off-by: Michael Huang <michaelhly@gmail.com>

Closes scylladb/scylladb#15481
2023-09-21 20:18:49 +03:00
Aleksandra Martyniuk
6c7eb7096e cql3: statements: delete execute override
Delete overriden create_table_statement::execute as it only calls its
direct parent's (schema_altering_statement) execute method anyway.
2023-09-21 13:24:26 +02:00
Aleksandra Martyniuk
60fdc44bce cql3: statements: call check_restricted_table_properties in prepare
Table properties validation is performed on statement execution.
Thus, when one attempts to create a table with invalid options,
an incorrect command gets committed in Raft. But then its
application fails, leading to a raft machine being stopped.

Check table properties when create and alter statements are prepared.

The error is no longer returned as an exceptional future, but it
is thrown. Adjust the tests accordingly.
2023-09-21 13:21:51 +02:00
Aleksandra Martyniuk
ec98b182c8 cql3: statements: pass data_dictionary::database to check_restricted_table_properties
Pass data_dictionary::database to check_restricted_table_properties
as an arguemnt instead of query_processor as the method will be called
from a context which does not have access to query processor.
2023-09-21 13:20:45 +02:00
Botond Dénes
844a0e426f Merge 'Mark counters with skip when empty' from Amnon Heiman
This series mark multiple high cardinality counters with skip_when_empty flag.
After this patch the following counters will not be reported if they were never used:
```
scylla_transport_cql_errors_total
scylla_storage_proxy_coordinator_reads_local_node
scylla_storage_proxy_coordinator_completed_reads_local_node
scylla_transport_cql_errors_total
```
Also marked, the CAS related CQL operations.
Fixes #12751

Closes scylladb/scylladb#13558

* github.com:scylladb/scylladb:
  service/storage_proxy.cc: mark counters with skip_when_empty
  cql3/query_processor.cc: mark cas related metrics with skip_when_empty
  transport/server.cc: mark metric counter with skip_when_empty
2023-09-19 15:02:39 +03:00
Kefu Chai
484d02da14 cql3: expr: do not use multi-line comment
do not use muti-line comment. this silences the warning from GCC:
```
In file included from ./cql3/prepare_context.hh:19,
                 from ./cql3/statements/raw/parsed_statement.hh:14,
                 from build/debug/gen/cql3/CqlParser.hpp:62,
                 from build/debug/gen/cql3/CqlParser.cpp:44:
./cql3/expr/expression.hh:490:1: error: multi-line comment [-Werror=comment]
  490 | /// Custom formatter for an expression.  Supports multiple modes:\
      | ^
```

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>

Closes scylladb/scylladb#15471
2023-09-19 12:00:09 +03:00
Avi Kivity
ab6988c52f Merge "auth: do not grant permissions to creator without actually creating" from Wojciech Mitros
Currently, when creating the table, permissions may be mistakenly
granted to the user even if the table is already existing. This
can happen in two cases:

The query has a IF NOT EXISTS clause - as a result no exception
is thrown after encountering the existing table, and the permission
granting is not prevented.
The query is handled by a non-zero shard - as a result we accept
the query with a bounce_to_shard result_message, again without
preventing the granting of permissions.
These two cases are now avoided by checking the result_message
generated when handling the query - now we only grant permissions
when the query resulted in a schema_change message.

Additionally, a test is added that reproduces both of the mentioned
cases.

CVE-2023-33972

Fixes #15467.

* 'no-grant-on-no-create' of github.com:scylladb/scylladb-ghsa-ww5v-p45p-3vhq:
  auth: do not grant permissions to creator without actually creating
  transport: add is_schema_change() method to result_message
2023-09-18 21:47:28 +03:00
Kefu Chai
cf5400bc75 cql.g: always initialize returned values
always initialize returned values. the branches which
return these unitiailized returned values handles the
unmatched cases, so this change should not have any
impact on the behavior.

ANTLR3's C++ code generator does not assign any value
to the value, if it runs into failure or encounter
exceptions. for instance, following rule assigns the
value of `isStatic` to `isStaticColumn` only if
nothing goes wrong.

```
cfisStatic returns [bool isStaticColumn]
    @init{
        bool isStatic = false;
    }
    : (K_STATIC { isStatic=true; })?
    {
        $isStaticColumn = isStatic;
    }
    ;
```

as shown in the generated C++ code:
```c++
                switch (alt118)
                {
            	case 1:
            	    // build/debug/gen/cql3/Cql.g:989:8: K_STATIC
            	    {
            	         this->matchToken(K_STATIC, &FOLLOW_K_STATIC_in_cfisStatic5870);
            	        if  (this->hasException())
            	        {
            	            goto rulecfisStaticEx;
            	        }
            	        if (this->hasFailed())
            	        {
            	            return isStaticColumn;
            	        }

            	        if ( this->get_backtracking()==0 )
            	        {
            	             isStaticColumn=isStatic;

            	        }

            	    }
            	    break;

                }
```

when `this->hasException()` or `this->hasFailed()`,
`isStaticColumn` is returned right away without being
initialized, because we don't assign any initial value
to it, neither do we customize the exception handling
for this rule.

and, the parser bails out when its smells something bad
after it tries to match the specified rule. also, the
parser is a stateful tokenizer, its failure state is
carried by the parser itself. also, the matchToken()
*could* fail when trying to find the matched token,
this is the runtime behavior of parser, that's why the
compiler cannot be certain that the error path won't
be taken.

anyway, let's always initialize the values without
default constructor. the return values whose type
is of scoped enum are initialized with zero
initialization, because their types don't provide an
"invalid" value.

this change should silence warnings like:

```
clang++ -MD -MT build/debug/gen/cql3/CqlParser.o -MF build/debug/gen/cql3/CqlParser.o.d -I/home/kefu/dev/scylladb/seastar/include -I/home/kefu/dev/scylladb/build/debug/seastar/gen/include -U_FORTIFY_SOURCE -DSEASTAR_SSTRING -Werror=unused-result -fstack-clash-protection -fsanitize=address -fsanitize=undefined -fno-sanitize=vptr -DSEASTAR_API_LEVEL=7 -DSEASTAR_BUILD_SHARED_LIBS -DSEASTAR_SCHEDULING_GROUPS_COUNT=16 -DSEASTAR_DEBUG -DSEASTAR_DEFAULT_ALLOCATOR -DSEASTAR_SHUFFLE_TASK_QUEUE -DSEASTAR_DEBUG_SHARED_PTR -DSEASTAR_LOGGER_TYPE_STDOUT -DSEASTAR_TYPE_ERASE_MORE -DBOOST_NO_CXX98_FUNCTION_BASE -DFMT_SHARED -I/usr/include/p11-kit-1   -ffile-prefix-map=/home/kefu/dev/scylladb=. -march=westmere -DDEBUG -DSANITIZE -DDEBUG_LSA_SANITIZER -DSCYLLA_ENABLE_ERROR_INJECTION -Og -DSCYLLA_BUILD_MODE=debug -g -gz -iquote. -iquote build/debug/gen --std=gnu++20  -ffile-prefix-map=/home/kefu/dev/scylladb=. -march=westmere  -DBOOST_TEST_DYN_LINK   -DNOMINMAX -DNOMINMAX -fvisibility=hidden  -Wall -Werror -Wextra -Wno-deprecated-copy -Wno-mismatched-tags -Wno-missing-field-initializers -Wno-c++11-narrowing -Wno-ignored-qualifiers -Wno-overloaded-virtual -Wno-unsupported-friend -Wno-unused-parameter -Wno-implicit-int-float-conversion -Wno-error=deprecated-declarations -DXXH_PRIVATE_API -DSEASTAR_TESTING_MAIN -DFMT_DEPRECATED_OSTREAM -Wno-parentheses-equality -O1 -fno-sanitize-address-use-after-scope -c -o build/debug/gen/cql3/CqlParser.o build/debug/gen/cql3/CqlParser.cpp
build/debug/gen/cql3/CqlParser.cpp:26645:28: error: variable 'perm' is uninitialized when used here [-Werror,-Wuninitialized]
                    return perm;
                           ^~~~
build/debug/gen/cql3/CqlParser.cpp:26616:5: note: variable 'perm' is declared here
    auth::permission perm;
    ^
build/debug/gen/cql3/CqlParser.cpp:52577:28: error: variable 'op' is uninitialized when used here [-Werror,-Wuninitialized]
                    return op;
                           ^~
build/debug/gen/cql3/CqlParser.cpp:52518:5: note: variable 'op' is declared here
    oper_t op;
    ^
```

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>

Closes scylladb/scylladb#15451
2023-09-18 16:45:50 +03:00
Aleksandra Martyniuk
ac08b57555 compaction: cql3: validate options in check_restricted_table_properties
Check whether valid compaction strategy options are set for the given
strategy type in check_restricted_table_properties.
2023-09-13 16:59:40 +02:00
Nadav Har'El
548386a0bb treewide: reduce include of cql_statement.hh
ClangBuildAnalyzer reports cql3/cql_statement.hh as being one of the
most expensive header files in the project - being included (mostly
indirectly) in 129 source files, and costing a total of 844 CPU seconds
of compilation.

This patch is an attempt, only *partially* successful, to reduce the
number of times that cql_statement.hh is included. It succeeds in
lowering the number 129 to 99, but not less :-( One of the biggest
difficulties in reducing it further is that query_processor.hh includes
a lot of templated code, which needs stuff from cql_statement.hh.
The solution should be to un-template the functions in
query_processor.hh and move them from the header to a source file, but
this is beyond the scope of this patch and query_processor.hh appears
problematic in other respects as well.

Unfortunately the compilation speedup by this patch is negligible
(the `du -bc build/dev/**/*.o` metric shows less than 0.01% reduction).
Beyond the fact that this patch only removes 30% of the inclusions of
this header, it appears that most of the source files that no longer
include cql_statement.hh after this patch, included anyway many of the
other headers that cql_statement.hh included, so the saving is minimal.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>

Closes #15212
2023-09-08 13:23:50 +03:00
Piotr Smaroń
eb46f1bd17 guardrails: restrict replication factor (RF)
Replacing `minimum_keyspace_rf` config option with 4 config options:
`{minimum,maximum}_replication_factor_{warn,fail}_threshold`, which
allow us to impose soft limits (issue a warning) and hard limits (not
execute CQL) on RF when creating/altering a keyspace.
The reason to rather replace than extend `minimum_keyspace_rf` config
option is to be aligned with Cassandra, which did the same, and has the
same parameters' names.
Only min soft limit is enabled by default and it is set to 3, which means
that we'll generate a CQL warning whenever RF is set to either 1 or 2.
RF's value of 0 is always allowed and means that there will not be any
replicas on a given DC. This was agreed with PM.
Because we don't allow to change guardrails' values when scylla is
running (per PM), there're no tests provided with this PR, and dtests will be
provided separately.
Exceeding guardrails' thresholds will be tracked by metrics.

Resolves #8619
Refs #8892 (the RF part, not the replication-strategy part)

Closes #14262
2023-09-04 19:22:17 +03:00
Amnon Heiman
c279409d48 cql3/query_processor.cc: mark cas related metrics with skip_when_empty
This patch mark the conditional metrics counters with skip_when_empty
flag, to reduce metrics reporting when cas is not in used.

Signed-off-by: Amnon Heiman <amnon@scylladb.com>
2023-08-23 09:30:35 -04:00
Gleb Natapov
4ffc39d885 cql3: Extend the scope of group0_guard during DDL statement execution
Currently we hold group0_guard only during DDL statement's execute()
function, but unfortunately some statements access underlying schema
state also during check_access() and validate() calls which are called
by the query_processor before it calls execute. We need to cover those
calls with group0_guard as well and also move retry loop up. This patch
does it by introducing new function to cql_statement class take_guard().
Schema altering statements return group0 guard while others do not
return any guard. Query processor takes this guard at the beginning of a
statement execution and retries if service::group0_concurrent_modification
is thrown. The guard is passed to the execute in query_state structure.

Fixes: #13942

Message-ID: <ZNsynXayKim2XAFr@scylladb.com>
2023-08-17 15:52:48 +03:00
Avi Kivity
d57a951d48 Revert "cql3: Extend the scope of group0_guard during DDL statement execution"
This reverts commit 70b5360a73. It generates
a failure in group0_test .test_concurrent_group0_modifications in debug
mode with about 4% probability.

Fixes #15050
2023-08-15 00:26:45 +03:00
Gleb Natapov
70b5360a73 cql3: Extend the scope of group0_guard during DDL statement execution
Currently we hold group0_guard only during DDL statement's execute()
function, but unfortunately some statements access underlying schema
state also during check_access() and validate() calls which are called
by the query_processor before it calls execute. We need to cover those
calls with group0_guard as well and also move retry loop up. This patch
does it by introducing new function to cql_statement class take_guard().
Schema altering statements return group0 guard while others do not
return any guard. Query processor takes this guard at the beginning of a
statement execution and retries if service::group0_concurrent_modification
is thrown. The guard is passed to the execute in query_state structure.

Fixes: #13942

Message-ID: <ZNSWF/cHuvcd+g1t@scylladb.com>
2023-08-13 14:19:39 +03:00
Kamil Braun
59c410fb97 Merge 'migration_manager: announce: provide descriptions for all calls' from Patryk Jędrzejczak
The `system.group0_history` table provides useful descriptions for each
command committed to Raft group 0. One way of applying a command to
group 0 is by calling `migration_manager::announce`. This function has
the `description` parameter set to empty string by default. Some calls
to `announce` use this default value which causes `null` values in
`system.group0_history`. We want `system.group0_history` to have an
actual description for every command, so we change all default
descriptions to reasonable ones.

Going further, We remove the default value for the `description`
parameter of `migration_manager::announce` to avoid using it in the
future. Thanks to this, all commands in `system.group0_history` will
have a non-null description.

Fixes #13370

Closes #14979

* github.com:scylladb/scylladb:
  migration_manager: announce: remove the default value of description
  test: always pass empty description to migration_manager::announce
  migration_manager: announce: provide descriptions for all calls
2023-08-09 16:58:41 +02:00
Patryk Jędrzejczak
27ddf78171 migration_manager: announce: provide descriptions for all calls
The system.group0_history table provides useful descriptions
for each command committed to Raft group 0. One way of applying
a command to group 0 is by calling migration_manager::announce.
This function has the description parameter set to empty string
by default. Some calls to announce use this default value which
causes null values in system.group0_history. We want
system.group0_history to have an actual description for every
command, so we change all default descriptions to reasonable ones.

We can't provide a reasonable description to announce in
query_processor::execute_thrift_schema_command because this
function is called in multiple situations. To solve this issue,
we add the description parameter to this function and to
handler::execute_schema_command that calls it.
2023-08-07 14:38:11 +02:00
Avi Kivity
4f7e83a4d0 cql3: select_statement: reject DISTINCT with GROUP BY on clustering keys
While in SQL DISTINCT applies to the result set, in CQL it applies
to the table being selected, and doesn't allow GROUP BY with clustering
keys. So reject the combination like Cassandra does.

While this is not an important issue to fix, it blocks un-xfailing
other issues, so I'm clearing it ahead of fixing those issues.

An issue is unmarked as xfail, and other xfails lose this issue
as a blocker.

Fixes #12479

Closes #14970
2023-08-07 15:35:59 +03:00
Pavel Emelyanov
595c5abbf9 wasm: Shuffle context::context()
Add a constructor that builds context out of const manager reference.
The existing one needs to get engine and instance cache and does it via
query_processor. This change lets removing those exports and finally --
drop the wasm::manager -> cql3::query_processor friendship

Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
2023-08-04 19:47:50 +03:00
Pavel Emelyanov
93cb73fddb wasm: Add manager::precompile()
This is not to make query_processor export alien runner from the
wasm::manager

Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
2023-08-04 19:47:50 +03:00
Pavel Emelyanov
d58a2d65b5 wasm: Move stop() out of query_processor
When the q.p. stops it also "stops" the wasm manager. Move this call
into main. The cql test env doesn't need this change, it stops the whole
sharded service which stops instances on its own

Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
2023-08-04 19:47:50 +03:00
Pavel Emelyanov
243f2217dd wasm: Make wasm sharded<manager>
The wasm::manager is just cql3::wasm_context renamed. It now sits in
lang/wasm* and is started as a sharded service in main (and cql test
env). This move also needs some headers shuffling, but it's not severe

This change is required to make it possible for the wasm::manager to be
shared (by reference) between q.p. and replica::database further

Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
2023-08-04 19:47:50 +03:00
Pavel Emelyanov
dde285e7e9 query_processor: Wrap wasm stuff in a struct
There are three wasm-only fields on q.p. -- engine, cache and runner.
This patch groups them on a single wasm_context structure to make it
earier to manipulate them in the next patches

The 'friend' declaration it temporary, will go away soon

Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
2023-08-04 19:47:50 +03:00
Piotr Dulikowski
fad1e82bf7 untyped_result_set: add missing_column_exception
Currently, when one tries to access a column that an untyped_result_set
does not contain, a `std::bad_variant_access` exception is thrown. This
exception's message provides very little context and it can be difficult
to even figure out where this message is coming from.

In order to improve the situation, a new exception `missing_column` is
introduced which includes the missing column's name in its error
message. The exception derives from `std::bad_variant_access` for
compatibility with existing code that may want to catch it.
2023-08-04 07:37:12 +02:00
Kamil Braun
84bb75ea0a Merge 'service: migration_manager: change the prepare_ methods to functions' from Patryk Jędrzejczak
The `migration_manager` service is responsible for schema convergence in
the cluster - pushing schema changes to other nodes and pulling schema
when a version mismatch is observed. However, there is also a part of
`migration_manager` that doesn't really belong there - creating
mutations for schema updates. These are the functions with `prepare_`
prefix. They don't modify any state and don't exchange any messages.
They only need to read the local database.

We take these functions out of `migration_manager` and make them
separate functions to reduce the dependency of other modules (especially
`query_processor` and CQL statements) on `migration_manager`. Since all
of these functions only need access to `storage_proxy` (or even only
`replica::database`), doing such a refactor is not complicated. We just
have to add one parameter, either `storage_proxy` or `database` and both
of them are easily accessible in the places where these functions are
called.

This refactor makes `migration_manager` unneeded in a few functions:
- `alternator::executor::create_keyspace`,
- `cql3::statements::alter_type_statement::prepare_announcement_mutations`,
- `cql3::statements::schema_altering_statement::prepare_schema_mutations`,
- `cql3::query_processor::execute_thrift_schema_command:`,
- `thrift::handler::execute_schema_command`.

We remove the `migration_manager&` parameter from all these functions.

Fixes #14339

Closes #14875

* github.com:scylladb/scylladb:
  cql3: query_processor::execute_thrift_schema_command: remove an unused parameter
  cql3: schema_altering_statement::prepare_schema_mutations: remove an unused parameter
  cql3: alter_type_statement::prepare_announcement_mutations: change parameters
  alternator: executor::create_keyspace: remove an unused parameter
  service: migration_manager: change the prepare_ methods to functions
2023-08-01 11:56:56 +02:00
Patryk Jędrzejczak
233d801f39 cql3: query_processor::execute_thrift_schema_command: remove an unused parameter
After changing the prepare_ methods of migration_manager to
functions, the migration_manager& parameter of
query_processor::execute_thrift_schema_command and
thrift::handler::execute_schema_command (that calls
query_processor::execute_thrift_schema_command) has been unused.
2023-08-01 10:07:31 +02:00