Add API for receiving asynchronous notices #147

vitaly-burovoy · 2017-05-26T00:41:54Z

Per request #144.

It seems I implemented it long time ago, but did not have enough time to finalize it as a PR.
I think a single callback per connection is enough, therefore API has just set_notice_callback instead of add_notice_callback and remove_notice_callback.

P.S.: Here is also two minor commits small enough to create separated PRs.

It is identical to the regular severity "S", but it is never localized. Introduced in Postgres-9.6: https://www.postgresql.org/docs/9.6/static/protocol-error-fields.html

1st1 · 2017-05-26T02:25:17Z

We already have add_lustener API, so add_notice_callback is the way to go.

vitaly-burovoy · 2017-05-26T11:10:50Z

I've just replaced the function to add_notice_callback and added remove_notice_callback.

elprans · 2017-05-26T20:37:44Z

asyncpg/protocol/protocol.pyx

+                    # Ignore new unknown fields
+                    pass
+
+            self.connection._notice(message)


A subclass of PostgresMessage should be returned here instead of a plain dict.

I thought that way, but why don't pass just a dict?
It is easier for logging (people don't need to iterate over an instance of a class), and callbacks don't get anything except a message consists of fields and values.

PostgresMessage is designed for exceptions, where it is reasonable to have all known fields as attributes, notices are lightweight, used at specified places (callbacks) and don't need so complex creation process (and reverse process -- iteration via attributes to create a dict to be logged).

A simple class is not much "heavier" than a dict. Furthermore, notices should be a rare occurrence (compared to Notify). I don't really think that the ability to enumerate fields is all that useful. On the other hand, if notices are instances of PostgresNotice object, we can make its __str__ default to message and provide access to the message dict via the as_dict() method. IMO, it's a much nicer interface than a plain dict.

elprans

LGTM, but please make it so that the notification callback receives an instance of PostgresMessage.

1st1 · 2017-05-26T20:46:19Z

asyncpg/connection.py

@@ -126,6 +127,24 @@ def __init__(self, protocol, transport, loop,
            del self._listeners[channel]
            await self.fetch('UNLISTEN {}'.format(channel))

+    def add_notice_callback(self, callback):
+        """Set a callback to be used when asyncronous NoticeResponse is received


The first line of the docstring should be a complete sentence describing what the method does.

Then an empty new line.

Then you can add more paragraphs.

1st1 · 2017-05-26T20:47:27Z

asyncpg/connection.py

@@ -909,6 +930,25 @@ def _cancel_current_command(self, waiter):

        self._loop.create_task(cancel())

+    def _notice(self, message):
+        # `self._notice_cb` is checked internally and passed here as the `cb`


Add a 'period' at the end of comment sentences. Also the comment looks outdated.

1st1 · 2017-05-26T20:48:21Z

asyncpg/connection.py

@@ -821,13 +840,15 @@ def is_closed(self):
        self._listeners = {}
        self._aborted = True
        await self._protocol.close()
+        self._notice_cb_set = set()


I'd rename it to _notice_callbacks.

1st1 · 2017-05-26T20:48:52Z

asyncpg/connection.py

@@ -909,6 +930,25 @@ def _cancel_current_command(self, waiter):

        self._loop.create_task(cancel())

+    def _notice(self, message):


I'd rename this to _schedule_notice_callbacks.

Disagree. They are called immediately, there is no schedule process.

Right. Well, you should use loop.call_soon anyways. We never want to call callback right away.

1st1 · 2017-05-26T20:49:21Z

asyncpg/protocol/protocol.pyx

+                    # Ignore new unknown fields
+                    pass
+
+            self.connection._notice(message)


1st1 · 2017-05-26T21:24:29Z

asyncpg/connection.py

+
+        for cb in self._notice_cb_set:
+            try:
+                cb(con_ref, message)


Use loop.call_soon here.

And we'll update notify to use call_soon in a separate PR. That's something that we just oversaw.

Sometimes it is useful to get all non-None members, usually for logging. Do it in a base class to avoid copy-pasting in users' code.

Notice message types are: WARNING, NOTICE, DEBUG, INFO, or LOG. https://www.postgresql.org/docs/current/static/protocol-error-fields.html Issue MagicStack#144.

1st1 · 2017-06-30T19:57:58Z

Hi @vitaly-burovoy! Are you going to continue working on this PR? We'll be releasing a new version of asyncpg soon.

vitaly-burovoy · 2017-07-05T03:58:24Z

I committed requested changes (I hope I did not forget something), but Travis could not check it because it failed installing Python in OSX (if I understand its log correctly).

What should I do now?

1st1 · 2017-07-05T15:22:52Z

I'm merging the PR although there's some work left; we'll open separate PRs to address it.

vitaly-burovoy · 2017-07-06T05:43:28Z

Thank you very much!

vitaly-burovoy added 2 commits May 25, 2017 14:03

Fix typo PooledConnectionProxy=>PoolConnectionProxy

10b73fc

New severity "V" field as "severity_en" attribute

b6cf4da

It is identical to the regular severity "S", but it is never localized. Introduced in Postgres-9.6: https://www.postgresql.org/docs/9.6/static/protocol-error-fields.html

vitaly-burovoy force-pushed the notices branch from 31067f2 to 171b1a1 Compare May 26, 2017 10:46

elprans self-requested a review May 26, 2017 20:34

elprans reviewed May 26, 2017

View reviewed changes

elprans requested changes May 26, 2017

View reviewed changes

1st1 requested changes May 26, 2017

View reviewed changes

1st1 reviewed May 26, 2017

View reviewed changes

vitaly-burovoy added 2 commits May 27, 2017 14:35

Implement "as_dict" method for PostgresMessage

30db879

Sometimes it is useful to get all non-None members, usually for logging. Do it in a base class to avoid copy-pasting in users' code.

Add API for receiving asynchronous notices

844804a

Notice message types are: WARNING, NOTICE, DEBUG, INFO, or LOG. https://www.postgresql.org/docs/current/static/protocol-error-fields.html Issue MagicStack#144.

vitaly-burovoy force-pushed the notices branch 2 times, most recently from 6934885 to 844804a Compare July 5, 2017 03:31

1st1 merged commit 1b1893d into MagicStack:master Jul 5, 2017

elprans mentioned this pull request Jul 6, 2017

Obtain RAISE NOTICE message #144

Closed

vitaly-burovoy deleted the notices branch December 5, 2017 04:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API for receiving asynchronous notices #147

Add API for receiving asynchronous notices #147

vitaly-burovoy commented May 26, 2017

1st1 commented May 26, 2017

vitaly-burovoy commented May 26, 2017

elprans May 26, 2017

1st1 May 26, 2017

vitaly-burovoy May 26, 2017

elprans May 26, 2017

elprans left a comment

1st1 May 26, 2017

1st1 May 26, 2017

1st1 May 26, 2017

1st1 May 26, 2017

vitaly-burovoy May 26, 2017

1st1 May 26, 2017

1st1 May 26, 2017

1st1 May 26, 2017

1st1 May 26, 2017

1st1 commented Jun 30, 2017

vitaly-burovoy commented Jul 5, 2017

1st1 commented Jul 5, 2017

vitaly-burovoy commented Jul 6, 2017

		@@ -909,6 +930,25 @@ def _cancel_current_command(self, waiter):

		self._loop.create_task(cancel())

		def _notice(self, message):

Add API for receiving asynchronous notices #147

Add API for receiving asynchronous notices #147

Conversation

vitaly-burovoy commented May 26, 2017

1st1 commented May 26, 2017

vitaly-burovoy commented May 26, 2017

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

elprans 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

1st1 commented Jun 30, 2017

vitaly-burovoy commented Jul 5, 2017

1st1 commented Jul 5, 2017

vitaly-burovoy commented Jul 6, 2017