Re: [PATCH] Function to get size of asynchronous notification queue

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(at)gmail(dot)com> wrote:

> On Wed, Jun 17, 2015 at 6:15 PM, Brendan Jurd <direvus(at)gmail(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

--
Gurjeet Singh http://gurjeet.singh.im/