Skip to content

sql: change multi-row bounded staleness errors to reference rows, not ranges#71473

Open
nvb wants to merge 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/boundedStalenessErr
Open

sql: change multi-row bounded staleness errors to reference rows, not ranges#71473
nvb wants to merge 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/boundedStalenessErr

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Oct 12, 2021

The number of ranges is an implementation detail. The condition for
bounded staleness reads to be accepted in v21.2 is that they touch up
to a single row.

Suggested by @rmloveland in cockroachdb/docs#11896.

… ranges

The number of ranges is an implementation detail. The condition for
bounded staleness reads to be accepted in v21.2 is that they touch up
to a single row.
@nvb nvb requested a review from rytaft October 12, 2021 16:40
@nvb nvb requested a review from a team as a code owner October 12, 2021 16:40
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

# there's no guarantee of that - we could have empty ranges.
statement error unimplemented: cannot use bounded staleness for queries that may touch more than one range or require an index join
statement error unimplemented: cannot use bounded staleness for queries that may touch more than one row or require an index join
SELECT * FROM t AS OF SYSTEM TIME with_max_staleness('1ms') WHERE k IS NULL LIMIT 1
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a case where the change is making things more confusing, instead of less. I can imagine users saying "I have a limit 1, so why isn't this working?". Does that seem like a concern to others as well? cc. @rmloveland @rytaft.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this a little when first seeing this error too. It seems difficult to explain briefly in a SQL error message that using LIMIT still means multiple rows are being touched and then filtered down to the limit count, but I can try to address from the docs side by updating cockroachdb/docs#11896 to make clear that LIMIT still means more rows are being touched

We have some other error messages that link to docs using our internal link shortener, perhaps that could be useful here as well. The link would be: https://www.cockroachlabs.com/docs/stable/follower-reads.html

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also filed cockroachdb/docs#11953 to address this possible misconception in our docs in a more general way (update LIMIT docs, various perf pages etc)

Copy link
Copy Markdown
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)


pkg/ccl/logictestccl/testdata/logic_test/as_of, line 223 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Also filed cockroachdb/docs#11953 to address this possible misconception in our docs in a more general way (update LIMIT docs, various perf pages etc)

Yea, this is the reason I used the word "range" instead of "row". But linking to the docs seems like a good solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants