Skip to content

Implement Send and Sync for PreparedQuery#445

Open
da-shalev wants to merge 1 commit into
Ralith:masterfrom
da-shalev:change-tracker-send-sync
Open

Implement Send and Sync for PreparedQuery#445
da-shalev wants to merge 1 commit into
Ralith:masterfrom
da-shalev:change-tracker-send-sync

Conversation

@da-shalev

@da-shalev da-shalev commented Jul 10, 2026

Copy link
Copy Markdown

PreparedQuery does not automatically implement Send and Sync because it retains pointer-capable fetch caches. Its prepared iterators and views already implement both traits when every Q::Item<'a> is Send; this applies the same bound to PreparedQuery itself.

ChangeTracker contains prepared queries, so it inherits Send + Sync transitively. This lets multithreaded schedulers store a ChangeTracker without introducing a local unsafe newtype.

The compile-time test covers prepared shared and mutable queries and confirms that ChangeTracker inherits both traits.

@Ralith

Ralith commented Jul 10, 2026

Copy link
Copy Markdown
Owner

Thanks, this is an oversight. In fact, I think these impls should probably be on PreparedQuery... do you see any scenario where that would be a problem?

While fixes to ChangeTracker and PreparedQuery are absolutely welcome, be aware that you don't need either of them; the former is implemented in terms of the public API, and the latter should almost never be faster than the now built-in query caching.

@da-shalev
da-shalev force-pushed the change-tracker-send-sync branch from 59b3d38 to 4f55d2a Compare July 10, 2026 21:19
@da-shalev da-shalev changed the title Implement Send and Sync for ChangeTracker Implement Send and Sync for PreparedQuery Jul 10, 2026
@da-shalev

da-shalev commented Jul 10, 2026

Copy link
Copy Markdown
Author

Thanks, this is an oversight. In fact, I think these impls should probably be on PreparedQuery... do you see any scenario where that would be a problem?

While fixes to ChangeTracker and PreparedQuery are absolutely welcome, be aware that you don't need either of them; the former is implemented in terms of the public API, and the latter should almost never be faster than the now built-in query caching.

You're right, I'm updating the PR to have the Send + Sync impl's directly on PreparedQuery. Having it on the ChangeTracker was the only thing I was concerned about because as you said PreparedQuery already caches itself.

Yes, thanks! I've looked into a event based impl of ChangeTracker.

The only issue I've ran into with event-based tracking is live change detection, so I explored using the provided ChangeTracker as a whole before over-optimizing with my own approach.

I'm exploring multiple forms of change detection before committing to one specific approach. And that's how I realized the oversight.

@da-shalev
da-shalev force-pushed the change-tracker-send-sync branch from ad9877f to 9007d8f Compare July 10, 2026 21:30
@da-shalev
da-shalev marked this pull request as ready for review July 10, 2026 21:31
Comment thread src/query.rs Outdated
}

unsafe impl<Q: Query> Send for PreparedQuery<Q> where for<'a> Q::Item<'a>: Send {}
unsafe impl<Q: Query> Sync for PreparedQuery<Q> where for<'a> Q::Item<'a>: Send {}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Are you sure the bound on Sync is correct? Bear in mind that all public methods are &mut. Also please include a safety comment.

@Ralith

Ralith commented Jul 11, 2026

Copy link
Copy Markdown
Owner

as you said PreparedQuery already caches itself.

To be clear: all queries are automatically cached. PreparedQuery is mostly redundant now.

@da-shalev
da-shalev force-pushed the change-tracker-send-sync branch from 9007d8f to e4b8910 Compare July 11, 2026 06:24
@da-shalev
da-shalev requested a review from Ralith July 11, 2026 08:06
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.

2 participants