Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

query_tombstone_page_limit applies to unpaged queries #17241

Closed
denesb opened this issue Feb 9, 2024 · 1 comment
Closed

query_tombstone_page_limit applies to unpaged queries #17241

denesb opened this issue Feb 9, 2024 · 1 comment
Assignees
Labels
Field-Tier1 Urgent issues as per FieldEngineering request
Milestone

Comments

@denesb
Copy link
Contributor

denesb commented Feb 9, 2024

The appropriate query-tombstone-page-limit (tombstone-limit) is selected based on the classification of the query, just like we do with the semaphores and the page limit. System queries have an infinite tombstone limit. Yet internal queries executed on behalf of user queries use the user limit. It was observed that in some cases this lead to internal queries into system_schema tables failing due to the tombstone limit affecting unpaged queries (internal queries are all unpaged).

The best solution to prevent internal queries from being affected is to make the tombstone-limit not affect unpaged queries at all. Having the tombstone limit affect unpaged queries doesn't make sense in the first place. The reason this limit was introduced was to prevent paged queries from timing out. It doesn't help unpaged queries, if anything this is counter productive, because unpaged queries which worked before, will fail with the tombstone limit and raising the limit can break paged queries.

@denesb denesb added this to the 6.0 milestone Feb 9, 2024
@denesb denesb self-assigned this Feb 9, 2024
denesb added a commit to denesb/scylla that referenced this issue Feb 9, 2024
The reason we introduced the tombstone-limit
(query_tombstone_page_limit), was to allow paged queries to return
incomplete/empty pages in the face of large tombstone spans. This works
by cutting the page after the tombstone-limit amount of tombstones were
processed. If the read is unpaged, it is killed instead. This was a
mistake. First, it doesn't really make sense, the reason we introduced
the tombstone limit, was to allow paged queries to process large
tombstone-spans without timing out. It does not help unpaged queries.
Furthermore, the tombstone-limit can kill internal queries done on
behalf of user queries, because all our internal queries are unpaged.
This can cause denial of service.

So in this patch we disable the tombstone-limit for unpaged queries
altogether, they are allowed to continue even after having processed the
configured limit of tombstones.

Fixes: scylladb#17241
@denesb denesb added backport/5.2 Issues that should be backported to 5.2 branch once they'll be fixed backport/5.4 Issues that should be backported to 5.4 branch once they'll be fixed labels Feb 9, 2024
@mykaul mykaul added Field-Tier1 Urgent issues as per FieldEngineering request Backport candidate labels Feb 9, 2024
denesb added a commit that referenced this issue Feb 15, 2024
The reason we introduced the tombstone-limit
(query_tombstone_page_limit), was to allow paged queries to return
incomplete/empty pages in the face of large tombstone spans. This works
by cutting the page after the tombstone-limit amount of tombstones were
processed. If the read is unpaged, it is killed instead. This was a
mistake. First, it doesn't really make sense, the reason we introduced
the tombstone limit, was to allow paged queries to process large
tombstone-spans without timing out. It does not help unpaged queries.
Furthermore, the tombstone-limit can kill internal queries done on
behalf of user queries, because all our internal queries are unpaged.
This can cause denial of service.

So in this patch we disable the tombstone-limit for unpaged queries
altogether, they are allowed to continue even after having processed the
configured limit of tombstones.

Fixes: #17241

Closes #17242

(cherry picked from commit f068d1a)
denesb added a commit that referenced this issue Feb 15, 2024
The reason we introduced the tombstone-limit
(query_tombstone_page_limit), was to allow paged queries to return
incomplete/empty pages in the face of large tombstone spans. This works
by cutting the page after the tombstone-limit amount of tombstones were
processed. If the read is unpaged, it is killed instead. This was a
mistake. First, it doesn't really make sense, the reason we introduced
the tombstone limit, was to allow paged queries to process large
tombstone-spans without timing out. It does not help unpaged queries.
Furthermore, the tombstone-limit can kill internal queries done on
behalf of user queries, because all our internal queries are unpaged.
This can cause denial of service.

So in this patch we disable the tombstone-limit for unpaged queries
altogether, they are allowed to continue even after having processed the
configured limit of tombstones.

Fixes: #17241

Closes #17242

(cherry picked from commit f068d1a)
@denesb
Copy link
Contributor Author

denesb commented Feb 15, 2024

Backported to 5.4 and 5.2.

@denesb denesb removed Backport candidate backport/5.2 Issues that should be backported to 5.2 branch once they'll be fixed backport/5.4 Issues that should be backported to 5.4 branch once they'll be fixed labels Feb 15, 2024
dgarcia360 pushed a commit to dgarcia360/scylla that referenced this issue Apr 30, 2024
The reason we introduced the tombstone-limit
(query_tombstone_page_limit), was to allow paged queries to return
incomplete/empty pages in the face of large tombstone spans. This works
by cutting the page after the tombstone-limit amount of tombstones were
processed. If the read is unpaged, it is killed instead. This was a
mistake. First, it doesn't really make sense, the reason we introduced
the tombstone limit, was to allow paged queries to process large
tombstone-spans without timing out. It does not help unpaged queries.
Furthermore, the tombstone-limit can kill internal queries done on
behalf of user queries, because all our internal queries are unpaged.
This can cause denial of service.

So in this patch we disable the tombstone-limit for unpaged queries
altogether, they are allowed to continue even after having processed the
configured limit of tombstones.

Fixes: scylladb#17241

Closes scylladb#17242
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Field-Tier1 Urgent issues as per FieldEngineering request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants