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

Log a warning when doing iter_by_col_range without an index #971

Merged
merged 3 commits into from
Mar 15, 2024

Conversation

gefjon
Copy link
Contributor

@gefjon gefjon commented Mar 14, 2024

Description of Changes

Adds a warning to iter_by_col_range when there is no applicable index and the table has >1000 rows. Should make it easier for us to debug BitCraft performance. This is an obviously suboptimal implementation which is intended as a stopgap until we can properly implement this warning.

Concerns:

  • Warnings go to the host logs, not the module logs. This will clearly need to change before 1.0.

Possible alternatives:

  • Simply don't generate filter_by_{col} methods unless there's an index.
    • This won't stop unindexed queries via the query! macro, but will catch the vast majority of cases.
    • This will not allow unindexed filters on small tables.
  • Generate the above methods, but mark them #[deprecated].
    • The point would not be to actually remove them, but because that's the only mechanism Rust exposes (AFAIK) for warning at a method's callsites.
    • This will warn on unindexed filters on small tables.
  • Generate the above methods, but with an ugly name, like slow_unindexed_filter_by_{col}.

Example

Using a modified version of the spacetimedb-quickstart module with a find_named reducer which does Person::filter_by_name. Note that I've snipped some calls; the table count is in fact accurate.

phoebe@phoebe-framework:~/clockworklabs/SpacetimeDB/modules/spacetimedb-quickstart]$ for i in {1..1000}; do spacetime call demo add "person-$i" 20; done

[phoebe@phoebe-framework:~/clockworklabs/SpacetimeDB/modules/spacetimedb-quickstart]$ spacetime call demo find_named person-1000
2024-03-14T13:56:59.291994Z  WARN iter_by_col_eq:iter_by_col_eq: crates/core/src/db/datastore/locking_tx_datastore/mut_tx.rs:1068: iter_by_col_range without index: table Person has 1021 rows; scanning columns ["name"]    

TODO:

  • Gate this behind a compile-time feature.
    • Should be enabled by default, but possible to disable so we can deploy the alpha without the logs.

API and ABI breaking changes

N/a.
[phoebe@phoebe-framework:~/clockworklabs/SpacetimeDB/modules/spacetimedb-quickstart]$ spacetime call demo list_over_age 20

Expected complexity level and risk

2 - social concern that we merge this and then never come back and write the better version.

@cloutiertyler
Copy link
Contributor

The one modification I would make is that we should have a threshold.

It's totally valid to do this if you have less than ~1000 elements.

So it really should be a "large table iter_by_col_range" warning.

Per Tyler's review, this commit gates the warning behind `rdb_num_table_rows`,
so that the warning is only printed
if the table in question has at least `TOO_MANY_ROWS_FOR_SCAN` rows.

`TOO_MANY_ROWS_FOR_SCAN` is defined as 1000
because that's the number Tyler said in his comment.
Copy link
Contributor

@kazimuth kazimuth left a comment

Choose a reason for hiding this comment

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

LGTM.

@mamcx
Copy link
Contributor

mamcx commented Mar 14, 2024

Possible alternatives:

  • Simply don't generate filter_by_{col} methods unless there's an index.

    • This won't stop unindexed queries via the query! macro, but will catch the vast majority of cases.
    • This will not allow unindexed filters on small tables.
  • Generate the above methods, but mark them #[deprecated].

Or better:

  • Indexes columns => filter_by_{col}, else => scan_by_{col}

Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

This looks great to me

@gefjon gefjon added this pull request to the merge queue Mar 15, 2024
Merged via the queue into master with commit 617b2a8 Mar 15, 2024
6 checks passed
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.

4 participants