Add support for Postgres notifications #4420

thkoch2001 · 2025-01-04T19:24:57Z

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

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.

weiznich

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.

weiznich · 2025-01-07T18:22:01Z

diesel/src/pg/connection/mod.rs

+        let (first, second, third) = {
+            let mut iter = conn.notifications_iter();
+            (iter.next().unwrap(), iter.next().unwrap(), iter.next())
+        };


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.

weiznich · 2025-01-07T18:23:20Z

diesel/src/pg/connection/mod.rs

@@ -547,6 +547,29 @@ impl PgConnection {
            .set_notice_processor(noop_notice_processor);
        Ok(())
    }
+
+    /// See Postgres documentation for SQL commands Notify and Listen


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.

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?

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.

weiznich · 2025-01-07T18:23:59Z

diesel/src/pg/connection/mod.rs

+#[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()
+    }
 }


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

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.

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

weiznich · 2025-01-07T18:25:19Z

diesel/src/pg/connection/raw.rs

+    /// optional data that was submitted with the notification, empty string if no data was submitted
+    pub payload: String,


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

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.

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?

weiznich · 2025-01-07T18:28:16Z

diesel/src/pg/connection/raw.rs

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


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.

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

I would rather use the DeserializationError variant for that.

weiznich · 2025-01-07T18:28:24Z

diesel/src/pg/connection/raw.rs

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


Suggested change

pub struct PGNotification {

pub struct PgNotification {

ups, sorry. Done.

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 requested changes Jan 7, 2025

View reviewed changes

weiznich force-pushed the add_pg_notifications branch 3 times, most recently from edc81cc to 3bb8841 Compare January 10, 2025 17:35

weiznich approved these changes Jan 10, 2025

View reviewed changes

weiznich marked this pull request as ready for review January 10, 2025 17:36

weiznich enabled auto-merge January 10, 2025 17:36

weiznich disabled auto-merge January 10, 2025 20:18

thkoch2001 and others added 4 commits January 11, 2025 10:25

Add support for Postgres notifications

896ef2d

This allows to receive notifications through Postgres' LISTEN/NOTIFY mechanism. diesel-rs#4418

fixed clippy and fmt issues

0f79ae6

adress review by @weiznich

d91ed27

weiznich force-pushed the add_pg_notifications branch from 3bb8841 to 59fa554 Compare January 11, 2025 09:28

weiznich enabled auto-merge January 11, 2025 09:28

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Postgres notifications #4420

Add support for Postgres notifications #4420

thkoch2001 commented Jan 4, 2025

weiznich left a comment

weiznich Jan 7, 2025

thkoch2001 Jan 8, 2025

weiznich Jan 7, 2025

thkoch2001 Jan 8, 2025

weiznich Jan 8, 2025

weiznich Jan 7, 2025

thkoch2001 Jan 8, 2025

weiznich Jan 8, 2025

weiznich Jan 7, 2025

thkoch2001 Jan 8, 2025

weiznich Jan 8, 2025

weiznich Jan 7, 2025

thkoch2001 Jan 8, 2025

weiznich Jan 8, 2025

weiznich Jan 7, 2025

thkoch2001 Jan 8, 2025

		/// optional data that was submitted with the notification, empty string if no data was submitted
		pub payload: String,

Add support for Postgres notifications #4420

Add support for Postgres notifications #4420

Conversation

thkoch2001 commented Jan 4, 2025

weiznich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment