perf(832): Remove redundant row deduplication in subscriptions #863
Conversation
joshua-spacetime
force-pushed
the
joshua/perf/832/subscription/row-dedup
branch
from
40686e7 to
0b011b5
Compare
|
|
||
| /// A set of [supported][`SupportedQuery`] [`QueryExpr`]s. | ||
| #[derive(Debug, Deref, DerefMut, PartialEq, From, IntoIterator)] | ||
| pub struct QuerySet(BTreeSet<SupportedQuery>); |
There was a problem hiding this comment.
Removed QuerySet in favor of ExecutionSet
| /// | ||
| /// NOTE: The returned `rows` in [DatabaseUpdate] are **deduplicated** so if 2 queries match the same `row`, only one copy is returned. | ||
| #[tracing::instrument(skip_all)] | ||
| pub fn eval_incr( |
There was a problem hiding this comment.
I have split the logic for eval and eval_incr depending on whether we are executing a single query or a batch of queries that return rows from the same table.
| type Error = DBError; | ||
|
|
||
| fn try_from(expr: QueryExpr) -> Result<Self, Self::Error> { | ||
| Ok(ExecutionSet::from_iter(vec![SupportedQuery::try_from(expr)?])) |
There was a problem hiding this comment.
| Ok(ExecutionSet::from_iter(vec![SupportedQuery::try_from(expr)?])) | |
| Ok(ExecutionSet::from_iter([SupportedQuery::try_from(expr)?])) |
| #[derive(Debug, PartialEq, Eq)] | ||
| pub struct ExecutionSet(BTreeSet<ExecOne>, BTreeSet<ExecMany>); | ||
|
|
||
| impl ExecutionSet { |
| let row_pk = pk_for_row(&row); | ||
|
|
||
| // Skip duplicate rows | ||
| if dup.contains(&row_pk) { |
There was a problem hiding this comment.
Wait, is possible to get the same row_pk by different tables? because the comment say it need to dedup by table, not by row.
There was a problem hiding this comment.
It is possible, but that's fine, because the same row in two different tables is considered two different rows.
| let mut queries = Vec::new(); | ||
| for sql in subscription.query_strings { | ||
| let qset = compile_read_only_query(&self.relational_db, tx, &auth, &sql)?; | ||
| queries.extend(qset); | ||
| } | ||
|
|
||
| let n = queries.len(); | ||
| let queries = ExecutionSet::from_iter(queries); |
There was a problem hiding this comment.
This could probably just be all one iter chain now.
|
@joshua-spacetime it'd probably be best to merge this first now that #888 has been reverted |
|
I could clean this up if that'd be easiest? |
|
@coolreader18 that would be great! You should be able to simplify this greatly now that we don't have to do any deduplication at all. |
coolreader18
force-pushed
the
joshua/perf/832/subscription/row-dedup
branch
3 times, most recently
from
e5863b1 to
d18afce
Compare
There was a problem hiding this comment.
@coolreader18 LGTM!
There are two tests that are failing
- test_subscribe_dedup
- test_subscribe_private
For (1) just remove it since we're no longer deduping. In (2) we call two queries that overlap
SELECT * FROM inventory
SELECT * FROM inventory WHERE inventory_id = 1
Just remove one of those queries from the test. Feel free to merge after that.
Closes #832. The database already operates under set semantics, so unless multiple queries return rows from the same table, deduplication of the result set is not necessary.
coolreader18
force-pushed
the
joshua/perf/832/subscription/row-dedup
branch
from
d18afce to
45d9ba4
Compare
Closes #832.
The database already operates under set semantics, so unless multiple queries return rows from the same table, deduplication of the result set is not necessary.
Performance numbers
Before
After