before this change, we construct a sstring from a comma statement,
which evaluates to the return value of `name.size()`, but what we
expect is `sstring(const char*, size_t)`.
in this change
* instead of passing the size of the string_view,
both its address and size are used
* `std::string_view` is constructed instead of sstring, for better
performance, as we don't need to perform a deep copy
the issue is reported by GCC-13:
```
In file included from cql3/selection/selectable.cc:11:
cql3/selection/field_selector.hh:83:60: error: ignoring return value of function declared with 'nodiscard' attribute [-Werror,-Wunused-result]
auto sname = sstring(reinterpret_cast<const char*>(name.begin(), name.size()));
^~~~~~~~~~
```
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#12666
This series contains the following changes for trimming the ranges passed to cleanup a compaction group to the compaction group owned token_range.
table: compaction_group_for_token: use signed arithmetic
Fixes#12595
table: make_compaction_groups: calculate compaction_group token ranges
table: perform_cleanup_compaction: trim owned ranges on compaction_group boundaries
Fixes#12594Closes#12598
* github.com:scylladb/scylladb:
table: perform_cleanup_compaction: trim owned ranges on compaction_group boundaries
table: make_compaction_groups: calculate compaction_group token ranges
dht: range_streamer: define logger as static
Currently, segment file removal first calls `f.remove_file()` and
does `total_size_on_disk -= f.known_size()` later.
However, `remove_file()` resets `known_size` to 0, so in effect
the freed space in not accounted for.
`total_size_on_disk` is not just a metric. It is also responsible
for deciding whether a segment should be recycled -- it is recycled
only if `total_size_on_disk - known_size < max_disk_size`.
Therefore this bug has dire performance consequences:
if `total_size_on_disk - known_size` ever exceeds `max_disk_size`,
the recycling of commitlog segments will stop permanently, because
`total_size_on_disk - known_size` will never go back below
`max_disk_size` due to the accounting bug. All new segments from this
point will be allocated from scratch.
The bug was uncovered by a QA performance test. It isn't easy to trigger --
it took the test 7 hours of constant high load to step into it.
However, the fact that the effect is permanent, and degrades the
performance of the cluster silently, makes the bug potentially quite severe.
The bug can be easily spotted with Prometheus as infinitely rising
`commitlog_total_size_on_disk` on the affected shards.
Fixes#12645Closes#12646
Since we're potentially searching the row_lock in parallel to acquiring the read_lock on the partition, we're racing with row_locker::unlock that may erase the _row_locks entry for the same clustering key, since there is no lock to protect it up until the partition lock has been acquired and the lock_partition future is resolved.
This change moves the code to search for or allocate the row lock _after_ the partition lock has been acquired to make sure we're synchronously starting the read/write lock function on it, without yielding, to prevent this use-after-free.
This adds an allocation for copying the clustering key in advance that wasn't needed before if the lock for it was already found, but the view building is not on the hot path so we can tolerate that.
This is required on top of 5007ded2c1 as seen in https://github.com/scylladb/scylladb/issues/12632 which is closely related to #12168 but demonstrates a different race causing use-after-free.
Fixes#12632
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Closes#12639
* github.com:scylladb/scylladb:
view: row_lock: lock_ck: try_emplace row_lock entry
view: row_lock: lock_ck: find or construct row_lock under partition lock
We currently don't clean up the system_distributed.view_build_status
table after removed nodes. This can cause false-positive check for
whether view update generation is needed for streaming.
The proper fix is to clean up this table, but that will be more
involved, it even when done, it might not be immediate. So until then
and to be on the safe side, filter out entries belonging to unknown
hosts from said table.
Fixes: #11905
Refs: #11836Closes#11860
Currently all consumed range tombstone changes are unconditionally
forwarded to the validator. Even if they are shadowed by a higher level
tombstone and/or purgable. This can result in a situation where a range
tombstone change was seen by the validator but not passed to the
consumer. The validator expects the range tombstone change to be closed
by end-of-partition but the end fragment won't come as the tombstone was
dropped, resulting in a false-positive validation failure.
Fix by only passing tombstones to the validator, that are actually
passed to the consumer too.
Fixes: #12575Closes#12578
The docs/alternator/compatibility.md file links to various open issues
on unimplemented features. One of the links was to an already-closed
issue. Replace it by a link to an open issue that was missing.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closes#12649
There's another one that accepts explicit basedir first argument and
that's used by the rest of the code.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closes#12643
We had one test test_gsi.py::test_gsi_identical that didn't work on KA/LA
sstables due to #6157, so it was skipped. Today, Scylla no longer supports
writing these old sstable formats, so the test can never find itself
running on these versions, so should pass. And indeed it does, and the
"skip" marker can be removed.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closes#12651
Today's sstable_test_env starts with a default-configured db::config and, thus, sstables_manager. Test cases that run in this env always create a tempdir to store sstable files in on their own. Next patching makes sstable-manager and friends fully control the data-dir path in order to support object storage for sstables in a nice way, and this behavior of tests upsets this ongoing work.
Said that, this PR configures sstable_test_env with a tempdir and pins down the cases using it to stick to that directory, rather than to the custom one.
Closes#12641
* github.com:scylladb/scylladb:
test: Use tempdir from sstable_test_env
test: Add tmpdir to sstable test env
test: Keep db::config as unique pointer
The leak sanitizer has a bug [1] where, if it detects a leak, it
forks something, and before that, it closes all files (instead of
using close_range like a good citizen).
Docker tends to create containers with the NOFILE limit (number of
open files) set to 1 billion.
The resulting 1 billion close() system calls is incredibly slow.
Work around that problem by passing the host NOFILE limit.
[1] https://github.com/llvm/llvm-project/issues/59112Closes#12638
Use same method as the two-level lock at the
partition level. try_emplace will either use
an existing entry, if found, or create a new
entry otherwise.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Since we're potentially searching the row_lock in parallel to acquiring
the read_lock on the partition, we're racing with row_locker::unlock
that may erase the _row_locks entry for the same clustering key, since
there is no lock to protect it up until the partition lock has been
acquired and the lock_partition future is resolved.
This change moves the code to search for or allocate the row lock
_after_ the partition lock has been acquired to make sure we're
synchronously starting the read/write lock function on it, without
yielding, to prevent this use-after-free.
This adds an allocation for copying the clustering key in advance
even if a row_lock entry already exists, that wasn't needed before.
It only us slows down (a bit) when there is contention and the lock
already existed when we want to go locking. In the fast path there
is no contention and then the code already had to create the lock
and copy the key. In any case, the penalty of copying the key once
is tiny compared to the rest of the work that view updates are doing.
This is required on top of 5007ded2c1 as
seen in https://github.com/scylladb/scylladb/issues/12632
which is closely related to #12168 but demonstrates a different race
causing use-after-free.
Fixes#12632
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
After topology changes like removing a node, verify that the set of
group 0 members and token ring members is the same.
Modify `get_token_ring_host_ids` to only return NORMAL members. The
previous version which used the `/storage_service/host_id` endpoint
might have returned non-NORMAL members as well.
Fixes: #12153Closes#12619
* seastar d41af8b59...943c09f86 (20):
> reactor: disable io_uring on older kernels if not enough lockable memory is available
> demos/tcp_sctp_client_demo: use user-defined literal for sizes
> core/units: add user-defined literal for IEC prefixes
> core/units: include what we use
> coroutine/exception: do not include core/coroutine.hh
> seastar/coroutine: drop std-coroutine.hh
> core/bitops.hh: add type constraits to templates
> apps/iotune: s/condition == false/!condition/
> core/metrics_api: s/promehteus/prometheus/
> reactor: make io_uring the default backend if available
> tests: connect_test: use 127.0.0.1 for connect refused test
> reactor: use aio to implement reactor_backend_uring::read()
> future: schedule: get_available_state_ref under SEASTAR_DEBUG
> rpc: client_info: add retrieve_auxiliary_opt
> Merge 'Make http requests with content-length header and generated body' from Pavel Emelyanov
> Merge 'Ensure logger doesn't allocate' from Travis Downs
> http, httpd: optimize header field assignment
> sstring: operator<< std::unordered_map: delete stray space char
> Dump memory diagnostics at error level on abort
> Fix CLI help for memory diagnostics dump
Closes#12650
There are several helpers to make an sstable for the table and two with most of the arguments are only used by tests. This PR leaves table with just one arg-less call thus making it easier to patch further.
Closes#12636
* github.com:scylladb/scylladb:
table: Shrink sstables making API
tests: Use sstables manager to make sstables
distributed_loader: Add helpers to make sstables for reshape/reshard
If a server is stopped suddenly (i.e. not graceful), schema tables might
be in inconsistent state. Add a test case and enable Scylla
configuration option (force_schema_commit_log) to handle this.
Fixes#12218Closes#12630
* github.com:scylladb/scylladb:
pytest: test start after ungraceful stop
test.py: enable force_schema_commit_log
The test cases in sstable_directory_test use a temporary directory that
differs from the one sstables manager starts over. Fix that.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
This adds the test/lib's tmpdir instance _and_ configures the
data_file_directories with this path. This makes sure sstables manager
and the rest of the test use the same directory for sstables. For now
it doesn't change anything, but helps next patching.
(A neat side effect of this change is that sstable_test_env is now
configured the same way as cql_test_env does)
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Currently there are four helpers, this patch makes it just two and one
of them becomes private the table thus making the API small and neat
(and easy to patch further).
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
This test uses two many-args helpers from table calss to create sstables
with desired parameters. The table API in question is not used by any
other code but these few places, to it's better to open-code it.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
This kills two birds with one stone. First, it factors out (quite a lot
of) common arguments that are passed to table.make_sstable(). Second, it
makes the helpers call sstable manager with extended args making it
possible to remove those wrappers from table class later.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Although the number of keyspaces should mostly be 1 here, and thus the
chance of two tables from different keyspaces colliding is miniscule, it
is not zero. Better be safe than sorry, so match the keyspace name too
when looking up a table.
Closes#12627
Each time backlog tracker is informed about a new or old sstable, it
will recompute the static part of backlog which complexity is
proportional to the total number of sstables.
On schema change, we're calling backlog_tracker::replace_sstables()
for each existing sstable, therefore it produces O(N ^ 2) complexity.
Fixes#12499.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Closes#12593
This phrase is inaccurate and unnecessary. We know all lines in the
printout are for reads and they are semaphores: no need to repeat this
information on each line.
Example:
Read Concurrency Semaphores:
read: 0/100, 0/ 41901096, queued: 0
streaming: 0/ 10, 0/ 41901096, queued: 0
system: 0/ 10, 0/ 41901096, queued: 0
Closes#12633
The dbuild README has an example how to enable ccache, and required
modifying the PATH. Since recently, our docker image includes
required commands (cxxbridge) in /usr/local/bin, so the build will
fail if that directory isn't also in the path - so add it in the
example.
Also use the opportunity to fix the "/home/nyh" in one example to
"$HOME".
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closes#12631
The goal is to make it possible to make config with custom-initialized
options in test_env::impl's constructor initializer list (next patch).
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
`ScyllaCluster.server_stop` had this piece of code:
```
server = self.running.pop(server_id)
if gracefully:
await server.stop_gracefully()
else:
await server.stop()
self.stopped[server_id] = server
```
We observed `stop_gracefully()` failing due to a server hanging during
shutdown. We then ended up in a state where neither `self.running` nor
`self.stopped` had this server. Later, when releasing the cluster and
its IPs, we would release that server's IP - but the server might have
still been running (all servers in `self.running` are killed before
releasing IPs, but this one wasn't in `self.running`).
Fix this by popping the server from `self.running` only after
`stop_gracefully`/`stop` finishes.
Make an analogous fix in `server_start`: put `server` into
`self.running` *before* we actually start it. If the start fails, the
server will be considered "running" even though it isn't necessarily,
but that is OK - if it isn't running, then trying to stop it later will
simply do nothing; if it is actually running, we will kill it (which we
should do) when clearing after the cluster; and we don't leak it.
Closes#12613
Test case for a start of a server after it was stopped suddenly (instead
of gracefully). This coud cause commitlog flush issues.
Signed-off-by: Alejo Sanchez <alejo.sanchez@scylladb.com>
New clusters that use a fresh conf/scylla.yaml will have `consistent_cluster_management: true`, which will enable Raft, unless the user explicitly turns it off before booting the cluster.
People using existing yaml files will continue without Raft, unless consistent_cluster_management is explicitly requested during/after upgrade.
Also update the docs: cluster creation and node addition procedures.
Fixes#12572.
Closes#12585
* github.com:scylladb/scylladb:
docs: mention `consistent_cluster_management` for creating cluster and adding node procedures
conf: enable `consistent_cluster_management` by default
Its _it member keeps state about the current range.
Although it's modified by the method, this is an implementation
detail that irrelevant to the caller, hence mark the
belongs_to_current_node method as const (and noexcept while
at it).
This allows the caller, cleanup_compaction, to use it from
inside a const method, without having to mark
its respective member as mutable too.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Closes#12634
change row purge condition for compacting_reader to remove all expired
rows to avoid read perfomance problems when there are many expired
tombstones in row cache
Refs #2252Closes#12565
From reviews of https://github.com/scylladb/scylladb/pull/12569, avoid
using `async with` and access the `Pool` of clusters with
`get()`/`put()`.
Closes#12612
* github.com:scylladb/scylladb:
test.py: manual cluster handling for PythonSuite
test.py: stop cluster if PythonSuite fails to start
test.py: minor fix for failed PythonSuite test
- makes all regexes static
If making regex compilation static
for uuid_type_impl and timeuuid_type_impl helps then it should
also help for timestamp_type and simple_date_type.
- remove unnecessary tolower transform in simple_date_type_impl::from_sstring
Following function uses only decimal and '-' characters (see date_re). They are not
affected by tolower call in any way.
Aditionally std::strtoll supports "0x" prefixes but also accepts
upper case version "0X" so it's also not affected by tolower call.
get_simple_date_time only casts strings to integer types using
boost:lexical_cast so also not affected by tolower.
Finally, serialize only uses str to include it in an exception text
so tolower doesn't affect it in a positive way. It's even better
that input is displayed to the user as it was, not converted to lower
case.
Closes#12621
* github.com:scylladb/scylladb:
types: remove unnecessary tolower transform in simple_date_type_impl::from_sstring
types: make all regexes static
Most of snitch drivers set _my_dc and _my_rack with direct assignment
thus skipping the sanity checks for dc/rack being empty. On other shards
they call set_my_dc_and_rack() helper which warns the empty value and
replaces it with some defaults.
It's better to use the helper on all shards in order to have the same
dc/rack values everywhere.
refs: #12185
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closes#12524
Issue #12538 suggested that maybe Alternator shouldn't bother reporting an
invalid table name in item operations like PutItem, and that it's enough
to report that the table doesn't exist. But the test added in this patch
shows that DynamoDB, like Alternator, reports the invalid table name in
this case - not just that the table doesn't exist.
That should make us think twice before acting on issue #12538. If we do
what this issue recommended, this test will need to be fixed (e.g., to
accept as correct both types of errors).
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closes#12608
before this change, we returns the total memory managed by Seastar
in the "total" field in system.memory. but this value only reflect
the total memory managed by Seastar's allocator. if
`reserve_additional_memory` is set when starting app_template,
Seastar's memory subsystem just reserves a chunk of memory of this
specified size for system, and takes the remaining memory. since
f05d612da8, we set this value to 50MB for wasmtime runtime. hence
the test of `TestRuntimeInfoTable.test_default_content` in dtest
fails. the test expects the size passed via the option of
`--memory` to be identical to the value reported by system.memory's
"total" field.
after this change, the "total" field takes the reserved memory
for wasm udf into account. the "total" field should reflect the total
size of memory used by Scylla, no matter how we use a certain portion
of the allocated memory.
Fixes#12522
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#12573
Instead of complex async with logic, use manual cluster pool handling.
Revert the discard() logic in Pool from a recent commit.
Signed-off-by: Alejo Sanchez <alejo.sanchez@scylladb.com>
Even though test can't fail both before and after, make the logic
explicit in case code changes in the future.
Signed-off-by: Alejo Sanchez <alejo.sanchez@scylladb.com>
add Make variable named `PREVIEW_HOST` so it can be overriden like
```
make preview PREVIEW_HOST=$(hostname -I | cut -d' ' -f 1)
```
it allows developer to preview the document if the host buiding the
document is not localhost.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#12589
It's a simple helper used during boot-time that can enjoy query-processor from sharded<system_keyspace>
Closes#12587
* github.com:scylladb/scylladb:
system_keyspace: De-static system_keyspace::increment_and_get_generation
system_keyspace: Fix indentation after previous patch
system_keyspace: Coroutinize system_keyspace::increment_and_get_generation
Following function uses only decimal and '-' characters (see date_re). They are not
affected by tolower call in any way.
Aditionally std::strtoll supports "0x" prefixes but also accepts
upper case version "0X" so it's also not affected by tolower call.
get_simple_date_time only casts strings to integer types using
boost:lexical_cast so also not affected by tolower.
Finally, serialize only uses str to include it in an exception text
so tolower doesn't affect it in a positive way. It's even better
that input is displayed to the user as it was, not converted to lower
case.
The reasons for force-disabling are doubly wrong: we now
use liburing from Fedora 37, which is sufficiently recent,
and the auto-detection code will disable io_uring if a
sufficiently recent version isn't present.
Closes#12620
