Commit 62458b8e4f introduced the enforcement of EXECUTE permissions of functions in cql select. However, according to the reference in #12869, the permissions should be enforced only on UDFs and UDAs.
The code does not distinguish between the two so the permissions are also unintenionally enforced also on native function. This commit introduce the distinction and only enforces the permissions on non native functions.
Fixes#16526
Manually verified (before and after change) with the reproducer supplied in #16526 and also with some the `min` and `max` native functions.
Also added test that checks for regression on native functions execution and verified that it fails on authorization before
the fix and passes after the fix.
Closesscylladb/scylladb#16556
* github.com:scylladb/scylladb:
test.py: Add test for native functions permissions
select statement: verify EXECUTE permissions only for non native functions
Commit 62458b8e4f introduced the
enforcement of EXECUTE permissions of functions in cql select. However,
according to the reference in #12869, the permissions should be enforced
only on UDFs and UDAs.
The code does not distinguish between the two so the permissions are
also unintentionally enforced also on native function.
This commit introduce the distinction and only enforces the permissions
on non native functions.
Fixes#16526
Manually verified (before and after change) with the reproducer
supplied in #16526 and also with some the `min` and `max` native
functions.
Signed-off-by: Eliran Sinvani <eliransin@scylladb.com>
This short series fixes a regression from Scylla 5.2 to Scylla 5.4 in "SELECT * GROUP BY" - this query was supposed to return just a single row from each partition (the first one in clustering order), but after the expression rewrite started to wrongly return all rows.
The series also includes a regression test that verifies that this query works doesn't work correctly before this series, but works with this patch - and also works as expected in Scylla 5.2 and in Cassadra.
Fixes#16531.
Closesscylladb/scylladb#16559
* github.com:scylladb/scylladb:
test/cql-pytest: check that most aggregators don't take "*"
cql-pytest: add reproducer for GROUP BY regression
cql: fix regression in SELECT * GROUP BY
Since we decided to drop CentOS7 support from latest version of Scylla, now we can drop CentOS7 specific codes from packaging scripts and setup scripts.
Related scylladb/scylla-enterprise#3502Closesscylladb/scylladb#16365
* github.com:scylladb/scylladb:
scylla-server.service: switch deprecated PermissionsStartsOnly to ExecStartPre=+
dist: drop legacy control group parameters
scylla-server.slice: Drop workaround for MemorySwapMax=0 bug
dist: move AmbientCapabilities to scylla-server.service
Revert "scylla_setup: add warning for CentOS7 default kernel"
[avi: CentOS 7 reached EOL on June 2024]
`--static-boost` is an option provided by `configure.py`. this option is
not used by our CI or building scripts. but in order to be compatible
with the existing behavior of `configure.py`, let's support this option
when building with CMake.
`Boost_USE_STATIC_LIBS` is a cmake variable supported by CMake's
FindBoost and Boost's own `BoostConfig.cmake`. see
https://cmake.org/cmake/help/latest/module/FindBoost.html#other-variables
by default boost is linked via its shared libraries. by setting
this variable, we link boost's static libraries.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#16545
scylla uses build modes like "debug" and "release" to differentiate
different build modes. while we intend to use the typical build
configurations / build types used by CMake like "Debug" and
"RelWithDebInfo" for naming CMAKE_CONFIGURATION_TYPES and
CMAKE_BUILD_TYPE. the former is used for naming the build directory and
for the preprocess macro named "SCYLLA_BUILD_MODE".
`test.py` and scylladb's CI are designed based on the naming of build
directory. in which, `test.py` lists the build modes using the dedicated
build target named `list_modes`, which is added by `configure.py`.
so, in this change, the target is added to CMake as well. the variables
of "scylla_build_mode" defined by the per-mode configuration are
collected and printed by the `list_modes`.
because, by default, CMake generates a target for each build
configuration when a multi-config generator is used. but we only want to
print the build mode for a single time when "list_modes" is built. so
a "BYPRODUCTS" is deliberately added for the target, and the patch of
this "BYPRODUCTS" is named without the "$<CONFIG>" it its path.
Closesscylladb/scylladb#16532
* github.com:scylladb/scylladb:
build: cmake: add "mode_list" target
build: cmake: define scylla_build_mode
when compiling clang-18 in "release" mode, `assert()` is optimized out.
so `i` is not used. and clang complains like:
```
/home/kefu/dev/scylladb/data_dictionary/user_types_metadata.hh:29:14: error: unused variable 'i' [-Werror,-Wunused-variable]
29 | auto i = _user_types.find(type->_name);
| ^
```
in this change, we use `i` as the hint for the insertion, for two
reasons:
- silence the warning.
- avoid the looking up in the unordered_map twice with the same
key.
`type` is not moved away when being passed to `insert_or_assign()`,
because otherwise, `type->_name` could be referencing a moved-away
shared_ptr, because the order of evaluating a function's parameter
is not determined. since `type` is a shared_ptr, the overhead is
negligible.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#16530
Although you can "SELECT COUNT(*)", this has special handling in the CQL
parser (it is converted into a special row-counting request) and you can't
give "*" to other aggregators - e.g., "SELECT SUM(*)". This patch includes
a simple test that confirms this.
I wanted to check this in relation to the previous patch, which did,
sort of, a "SELECT $$first$$(*)" - a syntax which this test shows
wouldn't have actually worked if we tried it.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
test/cql-pytest/test_group_by.py has tests that verifies that requests
like
SELECT p,c1,c2,v FROM tbl WHERE p=0 GROUP BY p
work as expected - the "GROUP BY p" means in this case that we should
only return the first row in the p=0 partition.
As a user discovered, it turns out that the almost identical request:
SELECT * FROM tbl WHERE p=0 GROUP BY p
Doesn't work the same - before the fix in the previous patch, it
erroneously returned all rows in p=0, not just the first one.
The test in this patch demonstrates this - it fails on Scylla 5.4,
passes on Scylla 5.2 and on Cassandra - and passes when the fix
from the previous patch is used.
This patch includes another tiny test, to check the interaction of GROUP BY
with filtering. This second test passes on Scylla - but I want it in
anyway because it is yet another interaction that might break (the
user that reported #16531 also had filtering, and I was worried it might
have been related).
Refs #16531
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Recently, the expression-rewrite effort changed the way that GROUP BY is
implemented. Usually GROUP BY involves an aggregation function (e.g., if
you want a separate SUM per partition). But there's also a query like
SELECT p, c1, c2, v FROM tbl GROUP BY p
This query is supposed to return one row - the *first* row in clustering
order - per group (in this case, partition). The expression rewrite
re-implemented this feature by introducing a new internal aggregator,
first(), which returns the first aggregated value. The above query is
rewritten into:
SELECT first(p), first(c1), first(c2), first(v) FROM tbl GROUP BY p
This case works correctly, and we even have a regression test for it.
But unfortunately the rewrite broke the following query:
SELECT * FROM tbl GROUP BY p
Note the "*" instead of the explicit list of columns.
In our implementation, a selection of "*" is looks like an empty
selection, and it didn't get the "first()" treatment and it remained
a "SELECT *" - and wrongly returned all rows instead of just the first
one in each partition. This was a regression - it worked correctly in
Scylla 5.2 (and also in Cassandra) - see the next patch for a
regression test.
In this patch we fix this regression. When there is a GROUP BY, the "*"
is rewritten to the appropriate list of all visible columns and then
gets the first() treatment, so it will return only the first row as
expected. The next patch will be a test that confirms the bug and its
fix.
Fixes#16531
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Right now the initial_tablets is kept as replication strategy option in the legacy system_schema.keyspaces table. However, r.s. options are all considered to be replication factors, not anything else. Other than being confusing, this also makes it impossible to extend keyspace configuration with non-integer tablets-related values.
This PR moves the initial_tablets into scylla-specific part of the schema. This opens a way to more ~~ugly~~ flexible ways of configuring tablets for keyspace, in particular it should be possible to use boolean on/off switch in CREATE KEYSPACE or some other trick we find appropriate.
Mos of what this PR does is extends arguments passed around keyspace_metadata and abstract_replication_strategy. The essence of the change is in last patches
* schema_tables: Relax extract_scylla_specific_ks_info() check
* locator,schema: Move initial tablets from r.s. options to params
refs: #16319
refs: #16364Closesscylladb/scylladb#16555
* github.com:scylladb/scylladb:
test: Add sanity tests for tablets initialization and altering
locator,schema: Move initial tablets from r.s. options to params
schema_tables: Relax extract_scylla_specific_ks_info() check
locator: Keep optional initial_tablets on r.s. params
ks_prop_defs: Add initial_tablets& arg to prepare_options()
keyspace_metadata: Carry optional<initial_tablets> on board
locator: Pass abstract_replication_strategy& into validate_tablet_options()
locator: Carry r.s. params into process_tablet_options()
locator: Call create_replication_strategy() with r.s. params
locator: Wrap replication_strategy_config_options into replication_strategy_params
locator: Use local members in ..._replication_strategy constructors
Check that the initial_tablets appears in system_schema.scylla_keyspaces
if turned on explicitly
Check that it's possible to change initial_tablets with ALTER KEYSPACE
Check that changing r.s. from simple to network-topology doesn't
activate tablets
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The option is kepd in DDL, but is _not_ stored in
system_schema.keyspaces. Instead, it's removed from the provided options
and kept in scylla_keyspaces table in its own column. All the places
that had optional initial_tablets disengaged now set this value up the
way the find appropriate.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Nowadays reading scylla-specific info from schema happens under
respective schema feature. However (at least in raft case) when a new
node joins the cluster merging schema for the first time may happen
_before_ features are merged and enabled. Thus merging schema can go the
wrong way by errorneously skipping the scylla-specific info.
On the other hand, if system_schema.scylla_keyspaces is there it's
there, there's no reason _not_ to pick this data up in that case.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Now all the callers have it at hands (spoiler: not yet initialized, but
still) so the params can also have it.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The prepare_options() method is in charge of pre-tuning the replication
strategy CQL parameters so that real keyspace and r.s. creation code
doesn't see some of those. The "initial_tablets" option is going to be
removed from the real options and be placed into scylla-specific part of
the schema. So the prepare_options() will need to modify both -- the
legacy options _and_ the (soon to be separate) initial_tablets thing.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The object in question fully describes the keyspace to be created and,
among other things, contains replication strategy options. Next patches
move the "initial_tablets" option out of those options and keep it
separately, so the ks metadata should also carry this option separately.
This patch is _just_ extending the metadata creation API, in fact the
new field is unused (write-only) so all the places that need to provide
this data keep it disengaged and are explicitly marked with FIXME
comment. Next patches will fix that.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The latter method is the one that will need extended params in next
patches. It's called from network_topology_strategy() constructor which
already has params at hand.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Previous patch added params to r.s. classes' constructors, but callers
don't construct those directly, instead they use the create_r.s.()
wrapper. This patch adds params to the wrapper too.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
When replication strategy class is created caller parr const reference
on the config options which is, in turn, a map<string, string>. In the
future r.s. classes will need to get "scylla specific" info along with
legacy options and this patch prepares for that by passing more generic
params argument into constructor. Currently the only inhabitant of the
new params is the legacy options.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The `config_options` arg had been used to initialize `_config_options`
field of the base abstract_replication_strategy class, so it's more
idiomatic to use the latter. Also it makes next patches simpler.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
When altering a keyspace several keyspace_metadata objects are created
along the way. The last one, that is then kept on the keyspace_metadata
object, forgets to get its copy of storage options thus transparently
converting to LOCAL type.
The bug surfaces itself when altering replication strategy class for
S3-backed storage -- the 2nd attempt fails, because after the 1st one
the keyspace_metadata gets LOCAL storage options and changing storage
options is not allowed.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#16524
b815aa021c added a yield before
the trace point, causing the moved `frozen_mutation_and_schema`
(and `inet_address_vector_topology_change`) to drop out of scope
and be destroyed, as the rvalue-referenced objects aren't moved
onto the coroutine frame.
This change passes them by value rather than by rvalue-reference
so they will be stored in the coroutine frame.
Fixes#16540
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Closesscylladb/scylladb#16541
The reader used to read the sstables was not closed. This could
sometimes trigger an abort(), because the reader was destroyed, without
it being closed first.
Why only sometimes? This is due to two factors:
* read_mutation_from_flat_mutation_reader() - the method used to extract
a mutation from the reader, uses consume(), which does not trigger
`set_close_is_required()` (#16520). Due to this, the top-level
combined reader did not complain when destroyed without close.
* The combined reader closes underlying readers who have no more data
for the current range. If the circumstances are just right, all
underlying readers are closed, before the combined reader is
destoyed. Looks like this is what happens for the most time.
This bug was discovered in SCT testing. After fixing #16520, all
invokations of `scylla-sstable`, which use this code would trigger the
abort, without this patch. So no further testing is required.
Fixes: #16519Closesscylladb/scylladb#16521
scylla uses build modes like "debug" and "release" to differentiate
different build modes. while we intend to use the typical build
configurations / build types used by CMake like "Debug" and
"RelWithDebInfo" for naming CMAKE_CONFIGURATION_TYPES and
CMAKE_BUILD_TYPE. the former is used for naming the build directory and
for the preprocess macro named "SCYLLA_BUILD_MODE".
`test.py` and scylladb's CI are designed based on the naming of build
directory. in which, `test.py` lists the build modes using the dedicated
build target named `list_modes`, which is added by `configure.py`.
so, in this change, the target is added to CMake as well. the variables
of "scylla_build_mode" defined by the per-mode configuration are
collected and printed by the `list_modes`.
because, by default, CMake generates a target for each build
configuration when a multi-config generator is used. but we only want to
print the build mode for a single time when "list_modes" is built. so
a "BYPRODUCTS" is deliberately added for the target, and the patch of
this "BYPRODUCTS" is named without the "$<CONFIG>" it its path.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
scylla uses build modes like "debug" and "release" to differentiate
different build modes. while we intend to use the typical build
configurations / build types used by CMake like "Debug" and
"RelWithDebInfo" for naming CMAKE_CONFIGURATION_TYPES and
CMAKE_BUILD_TYPE. the former is used for naming the build directory and
for the preprocess macro named "SCYLLA_BUILD_MODE".
`test.py` and scylladb's CI are designed based on the naming of build
directory. in which, `test.py` lists the build modes using the dedicated
build target named "list_modes", which is added by `configure.py`.
so, in this change, to prepare for adding the target,
"scylla_build_mode" is defined, so we can reuse it in a following-up
change.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
This switch is currently possible, but results in not supported keyspace state
Closesscylladb/scylladb#16513
* github.com:scylladb/scylladb:
test: Add a test that switching between vnodes and tablets is banned
cql3/statements: Don't allow switching between vnode and per-table replication strategies
cql3/statements: Keep local keyspace variable in alter_keyspace_statement::validate
This is a regression after #15903. Before these changes
del_leaving_endpoint took IP as a parameter and did nothing
if it was called with a non-existent IP.
The problem was revealed by the dtest test_remove_garbage_members_from_group0_after_abort_decommission[Announcing_that_I_have_left_the_ring-]. The test was
flaky as in most cases the node died before the
gossiper notification reached all the other nodes. To make
it fail consistently and reproduce the problem one
can move the info log 'Announcing that I have' after
the sleep and add additional sleep after it in
storage_service::leave_ring function.
Fixes#16466Closesscylladb/scylladb#16508
* seastar ae8449e04f...e0d515b6cf (18):
> reactor: poll less frequently in debug mode
> build: s/exec_program/execute_process/
> Merge 'httpd: support temporary redirect from inside async reply' from Noah Watkins
> Merge 'core: enable seastar to run multiple times in a single process' from Kefu Chai
> rpc/rpc_types: add formatter for rpc::optional<T>
> memory: do not set_reclaim_hook if cpu_mem_ptr is not set
> circleci: do not set disable dpdk explicitly
> fair_queue: Do not pop unplugged class immediately
> build: install Finducontext.cmake and FindSystem-SDT.cmake
> treewide: include used headers
> build: define SEASTAR_COROUTINES_ENABLED for Seastar module
> seastar.cc: include "core/prefault.hh"
> build: enable build C++20 modules with GCC 14
> build: replace seastar_supports_flag() with check_cxx_compiler_flag()
> Merge 'build: cleanups configure.py to be more PEP8 compatible' from Kefu Chai
> circleci: build with dpdk enabled
> build: add "--enable-cxx-modules" option to configure.py
> build: use a different *_CMAKE_API for CMake 3.27
Closesscylladb/scylladb#16500
Before this series, materialized views already work correctly on keyspaces with tablets, but secondary indexes do not. The goal of these series is make CQL secondary indexes fully supported on tablets:
1. First we need to make CREATE INDEX work with tablets (it didn't before this series). Fixes#16396.
2. Then we need to keep the promise that our documentation makes - that **local** secondary index should be synchronously updated - Fixes#16371.
As you can see in the patches below, and as was expected already in the design phase, the code changes needed to make indexes support tablets were minimal. But writing reliable tests for these issues was the biggest effort that went into this series.
Closesscylladb/scylladb#16436
* github.com:scylladb/scylladb:
secondary-index, tablets: ensure that LSI are synchronous
test: add missing "tags" schema extension to cql_test_env
mv, test: fix delay_before_remote_view_update injection point
secondary index: fix view creation when using tablets
The test test_many_partitions is very slow, as it tests a slow scan over
a lot of partitions. This was observed to time out on the slower ARM
machines, making the test flaky. To prevent this, create an
extra-patient cql connection with a 10 minutes timeout for the scan
itself.
This is a follow-up to fb9379edf1, which
attempted to fix this, but didn't patch all the places doing slow scans.
This patch fixes the other scan, the one actually observed to time-out
in CI.
Fixes: #16145Closesscylladb/scylladb#16370
When ALTER-ing a keyspace one may as well change its vnode/tablet
flavor, which is not currently supported, so prohibit this change
explicitly
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Both virtual tables and schema registry contain thread_local caches that are destroyed
at thread exit. after a Seastar change[1], these destructions can happen after the reactor
is destroyed, triggering a use-after-free.
Fix by scoping the destruction so it takes place earlier.
[1] 101b245ed7Closesscylladb/scylladb#16510
* github.com:scylladb/scylladb:
schema_registry, database: flush entries when no longer in use
virtual_tables: scope virtual tables registry in system_keyspace
The schema registry disarms internal timers when it is destroyed.
This accesses the Seastar reactor. However, after [1] we don't have ordering
between the reactor destruction and the thread_local registry destruction.
Fix this by flushing all entries when the database is destroyed. The
database object is fundamental so it's unlikely we'll have anything
using the registry after it's gone.
[1] 101b245ed7
Scylla skips exit hooks so we have to manually trigger the data dump to disk
from the LLVM profiling instrumentation runtime which we need in order
to support code coverage.
We use a weak symbol to get the address of the profile dump function. This
is legal: the function is a public interface of the instrumentation runtime.
Closesscylladb/scylladb#16430
Virtual tables are kept in a thread_local registry for deduplication
purposes. The problem is that thread_local variables are destroyed late,
possibly after the schema registry and the reactor are destroyed.
Currently this isn't a problem, but after a seastar change to
destroy the reactor after termination [1], things break.
Fix by moving the registry to system_keyspace. system_keyspace was chosen
since it was the birthplace of virtual tables.
Pimpl is used to avoid increasing dependencies.
[1] 101b245ed7
In other words, print more user-friendly messages, and avoid crashing.
Specifically:
* Don't crash when attempting to load schema tables from configured data-dir, while configuration does not have any configured data-directories.
* Detect the case where schema mutations have no rows for the current table -- the keyspace exists, but the table doesn't.
* Add negative tests for schema-loading.
Fixes: https://github.com/scylladb/scylladb/issues/16459Closesscylladb/scylladb#16494
* github.com:scylladb/scylladb:
test/cql-pytest: test_tools.py: add test for failed schema loadig
tools/scylla-sstable: use at() instead of operator [] when obtaining data dirs
tools/schema_loader: also check for empty table/column mutations
tools/schema_loader: log more details when loading schema from schema tables
truncating is an unusual operation, and we write a logging message
when the truncate op starts with INFO level, it would be great if
we can have a matching logging messge indicating the end of truncate
on the server side. this would help with investigation the TRUNCATE
timeout spotted on the client. at least we can rule out the problem
happening we server is performing truncate.
Refs #15610
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#16247
Consider this:
1) file streaming takes storage snapshot = list of sstables
2) concurrent compaction unlink some of those sstables from file system
3) file streaming tries to send unlinked sstables, but files other
than data and index cannot be read as only data and index have file
descriptors opened
To fix it, the snapshot now returns a set of files, one per sstable
component, for each sstable.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Closesscylladb/scylladb#16476
CQL Local Secondary Index is a Scylla-only extension to Cassandra's
secondary index API where the index is separate per partition.
Scylla's documentation guarantees that:
"As of Scylla Open Source 4.0, updates for local secondary indexes are
performed synchronously. When updates are synchronous, the client
acknowledges the write operation only after both the base table
modification and the view up date are written."
This happened automatically with vnodes, because the base table and the
view have the same partition key, so base and view replicas are co-located,
and the view update is always local and therefore done synchronously.
But with tablets, this does NOT happen automatically - the base and view
tablets may be located on different nodes, and the view update may be
remote, and NOT synchronous.
So in this patch we explicitly mark the view as synchronous_update when
building the view for an LSI.
The bigger part of this patch is to add a test which reliably fails
before this patch, and passes after it. The test creates a two-node
cluster and a table with LSI, and pins the base's tablets to one node
and the view's to the second node, forcing the view updates to be
remote. It also uses an injection point to make the view update slower.
The test then writes to the base and immediately tries to use the index
to read. Before this patch, the read doesn't find the new data (contrary
to the guarantee in the documentation). After this patch, the read
does find the new data - because the write waited for the index to
be updated.
Fixes#16371
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
