Re: [PATCH] Function to get size of asynchronous notification queue - Mailing list pgsql-hackers
| From | Gurjeet Singh |
|---|---|
| Subject | Re: [PATCH] Function to get size of asynchronous notification queue |
| Date | |
| Msg-id | CABwTF4Xy+q8Zby2fQ90BtY2N=chYCZqj90CVADo1a61grpHkmg@mail.gmail.com Whole thread Raw |
| In response to | Re: [PATCH] Function to get size of asynchronous notification queue (Merlin Moncure <mmoncure@gmail.com>) |
| Responses |
Re: [PATCH] Function to get size of asynchronous
notification queue
(Brendan Jurd <direvus@gmail.com>)
|
| List | pgsql-hackers |
Patch reviewed following the instructions on https://wiki.postgresql.org/wiki/Reviewing_a_Patch
# Submission review
- Is the patch in a patch format which has context?
Yes. Note to other reviewers: `git diff —patience` yields patch better suited for readability
- Does it apply cleanly to the current git master?
Yes.
- Does it include reasonable tests, necessary doc patches, etc?
Doc patch - Yes.
Tests - Yes.
# Usability review
- Does the patch actually implement the feature?
Yes.
- Do we want that?
Yes; see the discussion in mailing list.
- Do we already have it?
No.
- Does it follow SQL spec, or the community-agreed behavior?
Yes. It seems to implement the behavior agreed upon in the mailing list.
- Does it include pg_dump support (if applicable)?
N/A
- Are there dangers?
None that I could spot.
- Have all the bases been covered?
There’s room for an additional test which tests for non-zero return value.
# Feature test
- Does the feature work as advertised?
Yes. Build configured with '--enable-debug --enable-cassert CFLAGS=-O0’.
With a slightly aggressive notifications-in-a-loop script I was able to see non-zero return value:
Session 1:
listen ggg;
begin;
Session 2:
while sleep 0.1; do echo 'notify ggg; select pg_notification_queue_usage();' ; done | psql
- Are there corner cases the author has failed to consider?
No.
- Are there any assertion failures or crashes?
The patch exposes an already available function to the SQL interface, and rearranges code, so it doesn’t look like the patch can induce an assertion or a crash.
- Performance review
Not evaluated, since it’s not a performance patch, and it doesn’t seem to impact any performance critical code path, ,either.
Additional notes:
Patch updates the docs of another function (pg_listening_channels), but the update is an improvement so we can let it slide :)
+ proportion of the queue that is currently occupied by pending notifications.
s/proportion/fraction/
+ * The caller must hold (at least) shared AysncQueueLock.
A possibly better wording: The caller must hold AysncQueueLock in (at least) shared mode.
Unnecessary whitespace changes in pg_proc.h for existing functions.
+DESCR("get the current usage of the asynchronous notification queue");
A possibly better wording: get the fraction of the asynchronous notification queue currently in use
On Thu, Jun 18, 2015 at 10:47 AM, Merlin Moncure <mmoncure@gmail.com> wrote:
On Wed, Jun 17, 2015 at 6:15 PM, Brendan Jurd <direvus@gmail.com> wrote:
> Posting v2 of the patch, incorporating some helpful suggestions from Merlin.
Based on perfunctory scan of the code, I think this is gonna make it,
unless you draw some objections based on lack of necessity.
merlin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Gurjeet Singh http://gurjeet.singh.im/
pgsql-hackers by date: