Query paging API: hardening, robustness, explicitness #1061

wprzytula · 2024-08-20T11:00:59Z

Motivation

Query paging API has shown to be unclear for users:

{query,execute}_paged had misleading name (query_paged is a misleading name #940),
paging_state was a magic Bytes instance (Make paging state strongly typed #987),
{query,execute} would silently return only the first page if page size was specified on a statement,
users would naturally resort to {query,execute} as the methods with the simplest names, even though they should be used with SELECTs with caution:
with no page size set, an unpaged SELECT query would have high latency and memory footprint, and could event time out,
with page size set, only first page would be returned (see the previous big point).
it was easy to forget about iterating over pages based on paging_state, as it was just a field on QueryResult,
there was lack of examples of a loop together with {query,execute}_paged.

This PR seeks to address those problems before we attack the Giant Request Execution Refactor™ (GRER™) (#978).

What's done

The core idea incorporated:

Shift responsibility about whether to page or not from statements (by setting page_size to Some or None) to Session's methods.

Separate paged and unpaged API promotes conscious choice by users and thus prevents API misuse.

Detailed changes

Strongly typed PageSize is introduced, which restricts the inner i32's range to positive integers. This is important, because passing nonpositive integer as a page size silently makes the query unpaged. All APIs now accept PageSize, and it's user's responsibility to create a PageSize, handling possible errors thrown upon its construction.
Paging state is made strongly typed. Two new types are introduced: PagingState and PagingStateResponse. PagingState is, underneath, just an Option<Arc> over the raw bytes from the DB. The idea is that PagingState is passed as an argument to request execution API to ask for resuming the query (continuing it) from the point specified by given PagingState. PagingStateResponse is returned always by the DB from paged requests - with information about either more or no more pages being available. PagingStateResponse is easily convertible into PagingState, so that user can use it in in a loop.
PageSize is made mandatory on statements. Henceforth, paged API will always use it and unpaged API will always ignore it.
{query,execute} methods have _unpaged prefix appended and they always perform an unpaged query (ignoring the PageSize specified on a statement).
{query,execute}_paged are renamed to {query,execute}_single_page. The name %_paged has long confused users. The new name indicates explicitly that only a single page is fetched with a single call to those methods.
{query,execute}_single_page pass page state explicitly. As an attempt to make paged queries' API more explicit, robust and self-explanatory, PagingStateResponse is decoupled from QueryResult. Instead, it's returned from {query,execute}_single_page as a second field of a pair, helping users not to forget about it.
Moreover, internal and external APIs which expect that the query is not paged now warn if a "more pages" page state is returned from a query.
select-paging example is modified to showcase the suggested way to use {query,execute}_single_page.
Documentation is adjusted to the introduced changes.

Fixes: #940
Fixes: #987

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
~~[ ] I added relevant tests for new features and bug fixes.~~
All commits compile, pass static checks and pass test.
PR description sums up the changes and reasons why they should be introduced.
I have provided docstrings for the public items that I want to introduce.
I have adjusted the documentation in ./docs/source/.
I added appropriate Fixes: annotations to PR description.

github-actions · 2024-08-20T11:06:11Z

cargo semver-checks detected some API incompatibilities in this PR.
Checked commit: a1286de

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev 78286c3f7cbe5067d16c70597b4c87c582e0c5d9
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 78286c3f7cbe5067d16c70597b4c87c582e0c5d9
     Cloning 78286c3f7cbe5067d16c70597b4c87c582e0c5d9
     Parsing scylla v0.13.0 (current)
      Parsed [  21.567s] (current)
     Parsing scylla v0.13.0 (baseline)
      Parsed [  20.000s] (baseline)
    Checking scylla v0.13.0 -> v0.13.0 (no change)
     Checked [   0.076s] 79 checks: 77 pass, 2 fail, 0 warn, 0 skip

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.34.0/src/lints/inherent_method_missing.ron

Failed in:
  Session::query, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:620
  Session::query_paged, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:639
  Session::execute, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:973
  Session::execute_paged, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:987
  Session::query, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:620
  Session::query_paged, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:639
  Session::execute, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:973
  Session::execute_paged, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:987
  PreparedStatement::disable_paging, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/statement/prepared_statement.rs:161
  PreparedStatement::disable_paging, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/statement/prepared_statement.rs:161
  CachingSession::execute, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/caching_session.rs:72
  CachingSession::execute_paged, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/caching_session.rs:94
  Query::disable_paging, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/statement/query.rs:43
  Query::disable_paging, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/statement/query.rs:43

--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---

Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.34.0/src/lints/struct_pub_field_missing.ron

Failed in:
  field paging_state of struct QueryResult, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/query_result.rs:23
  field paging_state of struct QueryResult, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/query_result.rs:23

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  41.689s] scylla
     Parsing scylla-cql v0.2.0 (current)
      Parsed [   9.910s] (current)
     Parsing scylla-cql v0.2.0 (baseline)
      Parsed [   9.974s] (baseline)
    Checking scylla-cql v0.2.0 -> v0.2.0 (no change)
     Checked [   0.081s] 79 checks: 79 pass, 0 skip
     Summary no semver update required
    Finished [  20.014s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

scylla/src/transport/query_result.rs

scylla-cql/src/frame/request/query.rs

scylla/src/transport/connection.rs

scylla/src/transport/session.rs

muzarski

Some nits, otherwise LGTM

scylla/src/statement/mod.rs

docs/source/queries/result.md

Lorak-mmk

I don't like the first commit. Using invalid values for page size was never shown to be a problem / footgun for our users, so advantages from strong typing are quite limited. Otoh it introduces unnecessary complications in each call to set_page_size.
Imo in case of such obscure error, which also implies a bug in user code, a panic wuld be better - we still catch the error, but don't introduce complications for users.
So let's drop PageSize type and perform panicking check in set_page_size methods.

PR description is out of date - it mentions PagingContinuation.

In checklist you marked that All commits compile, pass static checks and pass test. - is that really the case? I would expect commit session: make {query,execute} methods unpaged to break something if it really prevents usage of unpaged queries. If it doesn't break any test then it sounds like our tests are lacking.

examples/select-paging.rs

scylla-cql/src/frame/request/query.rs

scylla/src/transport/connection.rs

wprzytula · 2024-08-26T09:59:05Z

So let's drop PageSize type and perform panicking check in set_page_size methods.

I suggest a slightly different thing: let's make PageSize not occur in the public API, but still be passed around in the internal codebase. This way we make sure that we won't forget about a panic anywhere - because we must handle an error on try_into() - I suggest unwrapping it.

wprzytula · 2024-08-26T10:02:23Z

In checklist you marked that All commits compile, pass static checks and pass test. - is that really the case? I would expect commit session: make {query,execute} methods unpaged to break something if it really prevents usage of unpaged queries. If it doesn't break any test then it sounds like our tests are lacking.

Good catch. Let's see if any test breaks.

Lorak-mmk · 2024-08-26T10:02:47Z

So let's drop PageSize type and perform panicking check in set_page_size methods.

I suggest a slightly different thing: let's make PageSize not occur in the public API, but still be passed around in the internal codebase. This way we make sure that we won't forget about a panic anywhere - because we must handle an error on try_into() - I suggest unwrapping it.

Makes sense

wprzytula · 2024-08-26T10:04:49Z

In checklist you marked that All commits compile, pass static checks and pass test. - is that really the case? I would expect commit session: make {query,execute} methods unpaged to break something if it really prevents usage of unpaged queries. If it doesn't break any test then it sounds like our tests are lacking.

Good catch. Let's see if any test breaks.

Sad news... everything has passed.

On a second thought, it's not strange that we haven't been testing improper behaviour. After all, query and execute should have never been used for paged queries, as they don't accept a non-start paging state. The only place that could potentially break is where query/execute was used as the first query before {query/execute}_paged loop, but apparently nothing broke.

wprzytula · 2024-08-26T13:54:16Z

v1.1: addressed all Karol's comments apart from PageSize panics, which is TODO.

wprzytula · 2024-08-26T14:31:49Z

v 1.2.1: mentioned that setters panic with non-positive numbers given.

Before, a bare i32 was passed. Some API methods would accept nonpositive integers, while others would panic on such input. Both those kinds of behaviour are bad, and the inconsistency between them is even worse. For robustness the popular Rust tactics is employed, namely strong typing. PageSize's public constructor returns error for nonpositive integers, ensuring that is an instance of that type exists, its validity is guaranteed. I originally wanted to put burden of creating a correct PageSize and handle possible errors on user, but is would introduce a boilerplate (`try_into().unwrap()`) that is deemed to be unacceptable. Therefore, PageSize is kept pub(crate) and it's constructed by driver's code, panicking on invalid user's input. As page size is always chosen by user explicitly (not by some convoluted logic), such panics should appear early and be clear to users.

The value of everything being empty does not seem to be a reasonable default value for both QueryResult and ResultMetadata. It's better to have a mock_empty() constructor for the specific case when such special value is needed.

As an effort of more robust paging API, paging state is now made typed. Two new types are introduced: PagingState and PagingStateResponse. PagingState is, underneath, just an Arc over the raw bytes from the DB. The idea is that PagingState is passed as an argument to request execution API to ask for resuming the query (_continue it_) from the given PagingState. PagingStateResponse is returned always by the DB from paged requests - with information about either more or no more pages available. PagingState can be easily retrieved from PagingStateResponse (if it contains some), so that user can use it in a loop. The select_paging example is modernised to showcase the idea.

As users' experience showed that it optional page size on statements is error-prone, it is made mandatory. This means that at the moment no unpaged queries are possible. However, in the next commits it is going to be brought back again.

Some methods on Connection return QueryResponse instead of QueryResult. To make those methods stand out, they get "_raw" particle put into their names.

Without a clear reason, Connection::execute_raw would take PreparedStatement by value, involving a clone. The signature was changed to accept a shared reference and its usages are adjusted.

These methods are analogous to `Session`'s `{query,execute}`. Similarly, they don't accept a non-start PagingState.

The logic of {query,execute} is extracted to {query_inner,execute_inner} pub(crate) methods, respectively. {query,execute}_paged are unchanged. {query,execute} now are unpaged unconditionally, i.e. they ignore the page size set on a statement and pass None to the connection layer. In the next commit, both {query,execute} are appended the `_unpaged` suffix to explicitly state that they perform unpaged queries.

In order to make it explicit that the requests using those methods are performed without paging, their names are adjusted.

As these methods are analogous to those on Session, they are made unpaged in a manner similar to those. Inthe next commit, both all of them are appended the `_unpaged` suffix to explicitly state that they perform unpaged queries.

In order to make it explicit that the requests using those methods are performed without paging, their names are adjusted.

In no case for internal queries should we fetch only one page and ignore possible further ones. As both queries that were using query_single_page would return only one row anyway, the change should not affect semantics after all.

The name %_paged has long confused users. The new name indicates explicitly that only a single page is fetched with a single call to those methods.

As an attempt to make paged queries' API more explicit, robust and self-explanatory, PagingState is decoupled from QueryResult. Instead, it's returned from {query,execute}_single_page as a second field of a pair, helping users not to forget about it. Moreover, internal and external APIs which expect that the query is not paged now issue an error tracing message and return ProtocolError error if a "more pages" page state is returned from the unpaged query.

It appears that query_single_page's previous semantics was to only fetch the first page and ignore others. It makes much more sense, though, to have its semantics consistent with Session::query_single_page: to take PagingState and query a single page pointed by that state. This allows using it in a loop, similarly to Session::query_single_page.

As {query,execute} methods got the _unpaged prefix, now the inner methods can drop the _inner prefix. This gives us one subtle advantage: users now see "method {query,execute} is private" instead of "{query,execute} method not found", which makes them more likely to think that the API was changed on purpose. Assuming that users see docstrings of private methods when they hover a call to them, this also lets them read the new docstrings of {query,execute} that explain the reasons behind the change and point to proper public methods: {query,execute}_{unpaged,single_page,iter}.

After the API changes regarding paging, docstrings are updated with those changes in mind.

wprzytula · 2024-08-27T09:56:16Z

v1.2.2: fully removed PageSize from public API - I forgot about getters - and made it pub(crate).

wprzytula requested review from Lorak-mmk, fee-mendes, muzarski and piodul August 20, 2024 11:01

wprzytula self-assigned this Aug 20, 2024

wprzytula added area/statement-execution area/documentation Improvements or additions to documentation labels Aug 20, 2024

github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Aug 20, 2024

wprzytula added this to the 0.14.0 milestone Aug 20, 2024

Lorak-mmk requested changes Aug 21, 2024

View reviewed changes

wprzytula force-pushed the paging-api-hardening branch from 0966edf to 76b1fae Compare August 22, 2024 16:58

wprzytula marked this pull request as draft August 22, 2024 16:58

wprzytula force-pushed the paging-api-hardening branch 4 times, most recently from 009283e to ee73b22 Compare August 23, 2024 09:39

wprzytula marked this pull request as ready for review August 23, 2024 09:39

wprzytula requested a review from Lorak-mmk August 23, 2024 09:39

muzarski approved these changes Aug 23, 2024

View reviewed changes

scylla/src/statement/mod.rs Outdated Show resolved Hide resolved

docs/source/queries/result.md Show resolved Hide resolved

wprzytula force-pushed the paging-api-hardening branch from ee73b22 to a7aa56f Compare August 26, 2024 06:56

Lorak-mmk mentioned this pull request Aug 26, 2024

Make heavy loaded optimization configurable #2 scylladb/gocql#211

Open

Lorak-mmk requested changes Aug 26, 2024

View reviewed changes

wprzytula force-pushed the paging-api-hardening branch from a7aa56f to 6248a1c Compare August 26, 2024 13:52

wprzytula force-pushed the paging-api-hardening branch from 6248a1c to 0fa3118 Compare August 26, 2024 13:55

wprzytula force-pushed the paging-api-hardening branch from f5b1839 to a139ca5 Compare August 26, 2024 14:31

wprzytula requested review from Lorak-mmk and muzarski August 26, 2024 14:33

wprzytula added 19 commits August 27, 2024 11:37

underive Default for QueryResult & ResultMetadata

22f558b

The value of everything being empty does not seem to be a reasonable default value for both QueryResult and ResultMetadata. It's better to have a mock_empty() constructor for the specific case when such special value is needed.

statement: make page size mandatory

6d9249f

As users' experience showed that it optional page size on statements is error-prone, it is made mandatory. This means that at the moment no unpaged queries are possible. However, in the next commits it is going to be brought back again.

connection: add _raw {suf/in}fix to relevant methods

5dc11b2

Some methods on Connection return QueryResponse instead of QueryResult. To make those methods stand out, they get "_raw" particle put into their names.

connection: execute_raw takes prepared by ref

e6f0268

Without a clear reason, Connection::execute_raw would take PreparedStatement by value, involving a clone. The signature was changed to accept a shared reference and its usages are adjusted.

connection: introduce non-raw query() and execute()

e415267

These methods are analogous to `Session`'s `{query,execute}`. Similarly, they don't accept a non-start PagingState.

connection: pass page size through request API

c008d76

session: add "_unpaged" suffix to {query,execute}

bfa853c

In order to make it explicit that the requests using those methods are performed without paging, their names are adjusted.

connection: make {query,execute}(_raw) methods unpaged

343b500

As these methods are analogous to those on Session, they are made unpaged in a manner similar to those. Inthe next commit, both all of them are appended the `_unpaged` suffix to explicitly state that they perform unpaged queries.

connection: append "_unpaged" to {query,execute}_{raw}

dfe79e9

In order to make it explicit that the requests using those methods are performed without paging, their names are adjusted.

connection: use query_unpaged for some internal queries

6ae5fa6

In no case for internal queries should we fetch only one page and ignore possible further ones. As both queries that were using query_single_page would return only one row anyway, the change should not affect semantics after all.

codewide: rename {query,execute}_paged to %_single_page

c5140c9

The name %_paged has long confused users. The new name indicates explicitly that only a single page is fetched with a single call to those methods.

session: update docstrings wrt paging

f82d85a

After the API changes regarding paging, docstrings are updated with those changes in mind.

docs: update for new paging API

a1286de

wprzytula force-pushed the paging-api-hardening branch from a139ca5 to a1286de Compare August 27, 2024 09:55

Lorak-mmk approved these changes Aug 27, 2024

View reviewed changes

muzarski approved these changes Aug 27, 2024

View reviewed changes

wprzytula merged commit 5752af4 into scylladb:main Aug 27, 2024

wprzytula deleted the paging-api-hardening branch August 27, 2024 10:10

wprzytula mentioned this pull request Aug 27, 2024

docs: Eradicate unpaged SELECTs and warn about their drawbacks #1068

Merged

5 tasks

Query paging API: hardening, robustness, explicitness #1061

Query paging API: hardening, robustness, explicitness #1061

Uh oh!

Conversation

wprzytula commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

What's done

The core idea incorporated:

Separate paged and unpaged API promotes conscious choice by users and thus prevents API misuse.

Detailed changes

Pre-review checklist

Uh oh!

github-actions bot commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

muzarski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Lorak-mmk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wprzytula commented Aug 26, 2024

Uh oh!

wprzytula commented Aug 26, 2024

Uh oh!

Lorak-mmk commented Aug 26, 2024

Uh oh!

wprzytula commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wprzytula commented Aug 26, 2024

Uh oh!

wprzytula commented Aug 26, 2024

Uh oh!

wprzytula commented Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wprzytula commented Aug 20, 2024 •

edited

Loading

github-actions bot commented Aug 20, 2024 •

edited

Loading

Lorak-mmk left a comment •

edited

Loading

wprzytula commented Aug 26, 2024 •

edited

Loading