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

Add support for Postgres notifications #4420

Merged
merged 4 commits into from
Jan 11, 2025

Conversation

thkoch2001
Copy link
Contributor

This allows to receive notifications through Postgres' LISTEN/NOTIFY mechanism.

#4418

I'm a rust newby, so please be frank about any possible errors I've made.

Especially I'm not sure whether I should somehow clone the internal_connection for the NotificationsIterator or if the user must drop the iterator before the connection can be used for other stuff.

I've not yet implemented blocking for new notifications as I don't need it and I think code should only be written when it is needed.

Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

Thanks for starting to work on this. This already looks great.

I've added a few comments about mostly stylistic and documentation related things. Let me know if you need help to address them

Before merging we should also add an Changelog.md entry for this as this is a relevant new feature.

Comment on lines 667 to 670
let (first, second, third) = {
let mut iter = conn.notifications_iter();
(iter.next().unwrap(), iter.next().unwrap(), iter.next())
};
Copy link
Member

Choose a reason for hiding this comment

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

Can we use let mut notifications = conn.notifications_iter().collect::<Vec<_>>() here instead and check that this vector contains exactly two elements. We could then try to fetch the next notification via conn.notifications_iter().next() to check that there is no notification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -547,6 +547,29 @@ impl PgConnection {
.set_notice_processor(noop_notice_processor);
Ok(())
}

/// See Postgres documentation for SQL commands Notify and Listen
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to link the postgres documentation here

It would be even better to have the code from the example as example code in the doc tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done adding the links to NOTIFY/LISTEN SQL commands documentation. However I couldn't build cargo doc --no-deps to test the formatting: "failed to run custom build command for mysqlclient-src v0.1.2". I guess I'd have to set up the build for mysql to. I already installed cmake but gave up on missing SASL and LDAP libs. And running rustdoc on just mod.rs fails on missing type information from other files.

I'm not sure what the benefit is of moving the example code here and making it a doctest:

  • I don't know how to ensure a db connection during doctest execution
  • the example code does not add coverage to the already existing test
  • rustdoc has Scraped examples functionality which should include the example in the docs, shouldn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, it should be possible to just run the Postgres tests via cargo xtask run-tests posgres without needing to care about any MySQL related dependencies. There is also a flag there to only run doc tests. If it doesn't work for you don't worry to much about this, I can take care of moving the example.

Scraped example unfortunately only work for in crate examples, not for such provided as external crate.

Comment on lines 560 to 596
#[allow(missing_debug_implementations)]
pub struct NotificationsIterator<'a> {
conn: &'a RawConnection,
}

pub use raw::PGNotification;

impl Iterator for NotificationsIterator<'_> {
type Item = PGNotification;

fn next(&mut self) -> Option<Self::Item> {
self.conn.pqnotifies()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer moving all notifications related code into a separate submodule to keep the code clean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With that I'd appreciate your help or better you doing it, please. It's much faster for you to just move the code where you want it. It's hard for me to balance everything what goes into API design and to predict your appreciation of the different factors.

Copy link
Member

Choose a reason for hiding this comment

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

I will take care of that in the next days, no worries

Comment on lines 219 to 220
/// optional data that was submitted with the notification, empty string if no data was submitted
pub payload: String,
Copy link
Member

Choose a reason for hiding this comment

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

If it's optional I would rather prefer to use a Option<String> here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

libpq does not provide any indication on whether NOTIFY was called with no payload or an empty string. Thus by adding an Option we would suggest the user we'd have more information than we actually do have and I can foresee bug reports from users that wonder why they received NONE when they did send an empty string.

Copy link
Member

Choose a reason for hiding this comment

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

In that case this seems to be the only reasonable approach to handle this. It's not great but we cannot do better if Postgres doesn't give us a way to differentiate beten empty and not set. Maybe the doc comment should include that information?


pub(super) fn pqnotifies(&self) -> Option<PGNotification> {
let conn = self.internal_connection;
let _ = unsafe { PQconsumeInput(conn.as_ptr()) };
Copy link
Member

Choose a reason for hiding this comment

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

we should check the returned integer here to check if an error occurred. That likely means that the notification iterator returns a QueryResult<PgNotification> instead of just a PgNotification>.

It should make it also easier to handle the possible UTF-8 errors below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how I could nicely convert the UTF-8 errors into DatabaseError.

Copy link
Member

Choose a reason for hiding this comment

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

I would rather use the DeserializationError variant for that.

// Using the same field names as tokio-postgres
/// See Postgres documentation for SQL Commands NOTIFY and LISTEN
#[derive(Clone, Debug)]
pub struct PGNotification {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub struct PGNotification {
pub struct PgNotification {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ups, sorry. Done.

@weiznich weiznich force-pushed the add_pg_notifications branch 3 times, most recently from edc81cc to 3bb8841 Compare January 10, 2025 17:35
@weiznich weiznich marked this pull request as ready for review January 10, 2025 17:36
@weiznich weiznich enabled auto-merge January 10, 2025 17:36
@weiznich weiznich disabled auto-merge January 10, 2025 20:18
thkoch2001 and others added 4 commits January 11, 2025 10:25
This allows to receive notifications through Postgres'
LISTEN/NOTIFY mechanism.

diesel-rs#4418
* Move the example as documentation example to the function to make it
  more discoverable
* Use `std::iter::from_fn` instead of a custom iterator type
* Move the `PgNotification` type to the backend module, as it is
  independend from the connection implementation (It might be reused
from diesel-async or similar crates)
* Improve error handling in the ffi wrapping function
   + Throw errors on null ptrs and non-UTF-8 strings
   + Make sure to always call `pqfree` on the notification, even if we
     return an error
@weiznich weiznich force-pushed the add_pg_notifications branch from 3bb8841 to 59fa554 Compare January 11, 2025 09:28
@weiznich weiznich enabled auto-merge January 11, 2025 09:28
@weiznich weiznich added this pull request to the merge queue Jan 11, 2025
Merged via the queue into diesel-rs:master with commit 2129cd9 Jan 11, 2025
49 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.

2 participants