Re: [HACKERS] [PATCH] Function to get size of asynchronous notification queue
On Fri, Jul 17, 2015 at 4:23 AM, Brendan Jurd dire...@gmail.com wrote: On Thu, 16 Jul 2015 at 08:37 Gurjeet Singh gurj...@singh.im wrote: OK. Please send a new patch with the changes you agree to, and I can mark it ready for committer. Done. Please find attached patch v3. I have changed proportion to fraction, and made other wording improvements per your suggestions. Speaking as a man whose children just finished fifth-grade math, a proportion, technically speaking, is actually a relationship between two fractions or ratios. So I agree that fraction is the right word here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Function to get size of asynchronous notification queue
On Thu, 16 Jul 2015 at 08:37 Gurjeet Singh gurj...@singh.im wrote:
OK. Please send a new patch with the changes you agree to, and I can mark
it ready for committer.
Done. Please find attached patch v3. I have changed proportion to
fraction, and made other wording improvements per your suggestions.
Cheers,
BJ
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 14806,14811 SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
--- 14806,14817
/row
row
+entryliteralfunctionpg_notification_queue_usage()/function/literal/entry
+entrytypedouble/type/entry
+entryfraction of the asynchronous notification queue currently occupied (0-1)/entry
+ /row
+
+ row
entryliteralfunctionpg_my_temp_schema()/function/literal/entry
entrytypeoid/type/entry
entryOID of session's temporary schema, or 0 if none/entry
***
*** 14945,14954 SET search_path TO replaceableschema/ optional, replaceableschema/, ..
primarypg_listening_channels/primary
/indexterm
para
functionpg_listening_channels/function returns a set of names of
! channels that the current session is listening to. See xref
! linkend=sql-listen for more information.
/para
indexterm
--- 14951,14969
primarypg_listening_channels/primary
/indexterm
+indexterm
+ primarypg_notification_queue_usage/primary
+/indexterm
+
para
functionpg_listening_channels/function returns a set of names of
! asynchronous notification channels that the current session is listening
! to. functionpg_notification_queue_usage/function returns the
! fraction of the total available space for notifications currently
! occupied by notifications that are waiting to be processed, as a
! typedouble/type in the range 0-1.
! See xref linkend=sql-listen and xref linkend=sql-notify
! for more information.
/para
indexterm
*** a/doc/src/sgml/ref/notify.sgml
--- b/doc/src/sgml/ref/notify.sgml
***
*** 166,171 NOTIFY replaceable class=PARAMETERchannel/replaceable [ , replaceable cla
--- 166,176
current transaction so that cleanup can proceed.
/para
para
+The function functionpg_notification_queue_usage/function returns the
+proportion of the queue that is currently occupied by pending notifications.
+See xref linkend=functions-info for more information.
+ /para
+ para
A transaction that has executed commandNOTIFY/command cannot be
prepared for two-phase commit.
/para
*** a/src/backend/commands/async.c
--- b/src/backend/commands/async.c
***
*** 371,376 static bool asyncQueueIsFull(void);
--- 371,377
static bool asyncQueueAdvance(volatile QueuePosition *position, int entryLength);
static void asyncQueueNotificationToEntry(Notification *n, AsyncQueueEntry *qe);
static ListCell *asyncQueueAddEntries(ListCell *nextNotify);
+ static double asyncQueueUsage(void);
static void asyncQueueFillWarning(void);
static bool SignalBackends(void);
static void asyncQueueReadAllNotifications(void);
***
*** 1362,1387 asyncQueueAddEntries(ListCell *nextNotify)
}
/*
! * Check whether the queue is at least half full, and emit a warning if so.
! *
! * This is unlikely given the size of the queue, but possible.
! * The warnings show up at most once every QUEUE_FULL_WARN_INTERVAL.
*
! * Caller must hold exclusive AsyncQueueLock.
*/
! static void
! asyncQueueFillWarning(void)
{
! int headPage = QUEUE_POS_PAGE(QUEUE_HEAD);
! int tailPage = QUEUE_POS_PAGE(QUEUE_TAIL);
! int occupied;
! double fillDegree;
! TimestampTz t;
occupied = headPage - tailPage;
if (occupied == 0)
! return; /* fast exit for common case */
if (occupied 0)
{
--- 1363,1399
}
/*
! * SQL function to return the fraction of the notification queue currently
! * occupied.
! */
! Datum
! pg_notification_queue_usage(PG_FUNCTION_ARGS)
! {
! double usage;
!
! LWLockAcquire(AsyncQueueLock, LW_SHARED);
! usage = asyncQueueUsage();
! LWLockRelease(AsyncQueueLock);
!
! PG_RETURN_FLOAT8(usage);
! }
!
! /*
! * Return the fraction of the queue that is currently occupied.
*
! * The caller must hold AysncQueueLock in (at least) shared mode.
*/
! static double
! asyncQueueUsage(void)
{
! int headPage = QUEUE_POS_PAGE(QUEUE_HEAD);
! int tailPage = QUEUE_POS_PAGE(QUEUE_TAIL);
! int occupied;
occupied = headPage - tailPage;
if (occupied == 0)
! return (double) 0; /* fast exit for common case */
if (occupied 0)
{
***
*** 1389,1396 asyncQueueFillWarning(void)
occupied += QUEUE_MAX_PAGE + 1;
}
! fillDegree = (double) occupied / (double) ((QUEUE_MAX_PAGE + 1) / 2);
if (fillDegree 0.5)
return;
--- 1401,1424
occupied += QUEUE_MAX_PAGE + 1;
}
Re: [HACKERS] [PATCH] Function to get size of asynchronous notification queue
On Fri, 17 Jul 2015 at 23:14 Robert Haas robertmh...@gmail.com wrote: Committed. I changed one remaining use of proportion to fraction, fixed an OID conflict, and reverted some unnecessary whitespace changes. Thanks Robert. Sorry I missed a proportion in my latest version, and thanks for catching it. Cheers, BJ
Re: [HACKERS] [PATCH] Function to get size of asynchronous notification queue
On Thu, Jun 25, 2015 at 8:43 PM, Brendan Jurd dire...@gmail.com wrote: On Fri, 26 Jun 2015 at 06:03 Gurjeet Singh gurj...@singh.im wrote: s/proportion/fraction/ I think of these as synonymous -- do you have any particular reason to prefer fraction? I don't feel strongly about it either way, so I'm quite happy to go with fraction if folks find that more expressive. It just feels better to me in this context. If the number of times used in Postgres code is any measure, 'fraction' wins hands down: proportion : 33, fraction: 620. I don't feel strongly about it, either. I can leave it up to the committer to decide. + * The caller must hold (at least) shared AysncQueueLock. A possibly better wording: The caller must hold AysncQueueLock in (at least) shared mode. Yes, that is more accurate. OK. Unnecessary whitespace changes in pg_proc.h for existing functions. I did group the asynchronous notification functions together, which seemed reasonable as there are now three of them, and changed the tabbing between the function name and namespace ID to match, as is done elsewhere in pg_proc.h. I think those changes improve readability, but again I don't feel strongly about it. Fair enough. +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 I have no objections to your wording. OK. Please send a new patch with the changes you agree to, and I can mark it ready for committer. Best regards, -- Gurjeet Singh http://gurjeet.singh.im/
Re: [HACKERS] [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 mmonc...@gmail.com wrote: On Wed, Jun 17, 2015 at 6:15 PM, Brendan Jurd dire...@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/
Re: [HACKERS] [PATCH] Function to get size of asynchronous notification queue
On Fri, 26 Jun 2015 at 06:03 Gurjeet Singh gurj...@singh.im wrote: Patch reviewed following the instructions on https://wiki.postgresql.org/wiki/Reviewing_a_Patch Thank you for your review, Gurjeet. s/proportion/fraction/ I think of these as synonymous -- do you have any particular reason to prefer fraction? I don't feel strongly about it either way, so I'm quite happy to go with fraction if folks find that more expressive. + * The caller must hold (at least) shared AysncQueueLock. A possibly better wording: The caller must hold AysncQueueLock in (at least) shared mode. Yes, that is more accurate. Unnecessary whitespace changes in pg_proc.h for existing functions. I did group the asynchronous notification functions together, which seemed reasonable as there are now three of them, and changed the tabbing between the function name and namespace ID to match, as is done elsewhere in pg_proc.h. I think those changes improve readability, but again I don't feel strongly about it. +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 I have no objections to your wording. Cheers, BJ
Re: [HACKERS] [PATCH] Function to get size of asynchronous notification queue
On Wed, Jun 17, 2015 at 6:15 PM, Brendan Jurd dire...@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
Re: [HACKERS] [PATCH] Function to get size of asynchronous notification queue
On Thu, 18 Jun 2015 at 03:06 Gurjeet Singh gurj...@singh.im wrote: I don't see this in the CF app; can you please add it there? Done. I did try to add it when I posted the email, but for some reason I couldn't connect to commitfest.postgresql.org at all. Seems fine now, though. Cheers, BJ
Re: [HACKERS] [PATCH] Function to get size of asynchronous notification queue
Posting v2 of the patch, incorporating some helpful suggestions from Merlin.
Cheers,
BJ
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 14800,14805 SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
--- 14800,14811
/row
row
+entryliteralfunctionpg_notification_queue_usage()/function/literal/entry
+entrytypedouble/type/entry
+entryproportion of the asynchronous notification queue currently occupied (0-1)/entry
+ /row
+
+ row
entryliteralfunctionpg_my_temp_schema()/function/literal/entry
entrytypeoid/type/entry
entryOID of session's temporary schema, or 0 if none/entry
***
*** 14939,14948 SET search_path TO replaceableschema/ optional, replaceableschema/, ..
primarypg_listening_channels/primary
/indexterm
para
functionpg_listening_channels/function returns a set of names of
! channels that the current session is listening to. See xref
! linkend=sql-listen for more information.
/para
indexterm
--- 14945,14963
primarypg_listening_channels/primary
/indexterm
+indexterm
+ primarypg_notification_queue_usage/primary
+/indexterm
+
para
functionpg_listening_channels/function returns a set of names of
! asynchronous notification channels that the current session is listening
! to. functionpg_notification_queue_usage/function returns the
! proportion of the total available space for notifications currently
! occupied by notifications that are waiting to be processed, as a
! typedouble/type in the range 0-1.
! See xref linkend=sql-listen and xref linkend=sql-notify
! for more information.
/para
indexterm
*** a/doc/src/sgml/ref/notify.sgml
--- b/doc/src/sgml/ref/notify.sgml
***
*** 166,171 NOTIFY replaceable class=PARAMETERchannel/replaceable [ , replaceable cla
--- 166,176
current transaction so that cleanup can proceed.
/para
para
+The function functionpg_notification_queue_usage/function returns the
+proportion of the queue that is currently occupied by pending notifications.
+See xref linkend=functions-info for more information.
+ /para
+ para
A transaction that has executed commandNOTIFY/command cannot be
prepared for two-phase commit.
/para
*** a/src/backend/commands/async.c
--- b/src/backend/commands/async.c
***
*** 371,376 static bool asyncQueueIsFull(void);
--- 371,377
static bool asyncQueueAdvance(volatile QueuePosition *position, int entryLength);
static void asyncQueueNotificationToEntry(Notification *n, AsyncQueueEntry *qe);
static ListCell *asyncQueueAddEntries(ListCell *nextNotify);
+ static double asyncQueueUsage(void);
static void asyncQueueFillWarning(void);
static bool SignalBackends(void);
static void asyncQueueReadAllNotifications(void);
***
*** 1362,1387 asyncQueueAddEntries(ListCell *nextNotify)
}
/*
! * Check whether the queue is at least half full, and emit a warning if so.
! *
! * This is unlikely given the size of the queue, but possible.
! * The warnings show up at most once every QUEUE_FULL_WARN_INTERVAL.
*
! * Caller must hold exclusive AsyncQueueLock.
*/
! static void
! asyncQueueFillWarning(void)
{
! int headPage = QUEUE_POS_PAGE(QUEUE_HEAD);
! int tailPage = QUEUE_POS_PAGE(QUEUE_TAIL);
! int occupied;
! double fillDegree;
! TimestampTz t;
occupied = headPage - tailPage;
if (occupied == 0)
! return; /* fast exit for common case */
if (occupied 0)
{
--- 1363,1399
}
/*
! * SQL function to return the proportion of the notification queue currently
! * occupied.
! */
! Datum
! pg_notification_queue_usage(PG_FUNCTION_ARGS)
! {
! double usage;
!
! LWLockAcquire(AsyncQueueLock, LW_SHARED);
! usage = asyncQueueUsage();
! LWLockRelease(AsyncQueueLock);
!
! PG_RETURN_FLOAT8(usage);
! }
!
! /*
! * Return the proportion of the queue that is currently occupied.
*
! * The caller must hold (at least) shared AysncQueueLock.
*/
! static double
! asyncQueueUsage(void)
{
! int headPage = QUEUE_POS_PAGE(QUEUE_HEAD);
! int tailPage = QUEUE_POS_PAGE(QUEUE_TAIL);
! int occupied;
occupied = headPage - tailPage;
if (occupied == 0)
! return (double) 0; /* fast exit for common case */
if (occupied 0)
{
***
*** 1389,1396 asyncQueueFillWarning(void)
occupied += QUEUE_MAX_PAGE + 1;
}
! fillDegree = (double) occupied / (double) ((QUEUE_MAX_PAGE + 1) / 2);
if (fillDegree 0.5)
return;
--- 1401,1424
occupied += QUEUE_MAX_PAGE + 1;
}
! return (double) occupied / (double) ((QUEUE_MAX_PAGE + 1) / 2);
! }
!
! /*
! * Check whether the queue is at least half full, and emit a warning if so.
! *
! * This is unlikely given the size of the queue, but possible.
!
Re: [HACKERS] [PATCH] Function to get size of asynchronous notification queue
On Thu, 18 Jun 2015 at 08:19 Merlin Moncure mmonc...@gmail.com wrote: scratch that. that note already exists in sql-notify.html. Instead, I'd modify that section to note that you can check queue usage with your new function. I have already done so. Under the paragraph about the queue filling up, I have added: The function functionpg_notify_queue_saturation/function returns the proportion of the queue that is currently occupied by pending notifications. A link from here back to the section in System Information Functions might be sensible? I will rename the function with _usage as you suggest, and add the explanation of the return value in the docs. Cheers, BJ
[HACKERS] [PATCH] Function to get size of asynchronous notification queue
Hello hackers,
I present a patch to add a new built-in function
pg_notify_queue_saturation().
The purpose of the function is to allow users to monitor the health of
their notification queue. In certain cases, a client connection listening
for notifications might get stuck inside a transaction, and this would
cause the queue to keep filling up, until finally it reaches capacity and
further attempts to NOTIFY error out.
The current documentation under LISTEN explains this possible gotcha, but
doesn't really suggest a useful way to address it, except to mention that
warnings will show up in the log once you get to 50% saturation of the
queue. Unless you happen to be eyeballing the logs when it happens, that's
not a huge help. The choice of 50% as a threshold is also very much
arbitrary, and by the time you hit 50% the problem has likely been going on
for quite a while. If you want your nagios (or whatever) to say, alert you
when the queue goes over 5% or 1%, your options are limited and awkward.
The patch has almost no new code. It makes use of the existing logic for
the 50% warning. I simply refactored that logic into a separate function
asyncQueueSaturation, and then added pg_notify_queue_saturation to make
that available in SQL.
I am not convinced that pg_notify_queue_saturation is the best possible
name for this function, and am very much open to other suggestions.
The patch includes documentation, a regression test and an isolation test.
Cheers,
BJ
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 14800,14805 SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
--- 14800,14811
/row
row
+entryliteralfunctionpg_notify_queue_saturation()/function/literal/entry
+entrytypedouble/type/entry
+entryproportion of the asynchronous notification queue currently occupied/entry
+ /row
+
+ row
entryliteralfunctionpg_my_temp_schema()/function/literal/entry
entrytypeoid/type/entry
entryOID of session's temporary schema, or 0 if none/entry
***
*** 14939,14948 SET search_path TO replaceableschema/ optional, replaceableschema/, ..
primarypg_listening_channels/primary
/indexterm
para
functionpg_listening_channels/function returns a set of names of
! channels that the current session is listening to. See xref
! linkend=sql-listen for more information.
/para
indexterm
--- 14945,14962
primarypg_listening_channels/primary
/indexterm
+indexterm
+ primarypg_notify_queue_saturation/primary
+/indexterm
+
para
functionpg_listening_channels/function returns a set of names of
! asynchronous notification channels that the current session is listening
! to. functionpg_notify_queue_saturation/function returns the proportion
! of the total available space for notifications currently occupied by
! notifications that are waiting to be processed. See
! xref linkend=sql-listen and xref linkend=sql-notify
! for more information.
/para
indexterm
*** a/doc/src/sgml/ref/notify.sgml
--- b/doc/src/sgml/ref/notify.sgml
***
*** 166,171 NOTIFY replaceable class=PARAMETERchannel/replaceable [ , replaceable cla
--- 166,175
current transaction so that cleanup can proceed.
/para
para
+The function functionpg_notify_queue_saturation/function returns the
+proportion of the queue that is currently occupied by pending notifications.
+ /para
+ para
A transaction that has executed commandNOTIFY/command cannot be
prepared for two-phase commit.
/para
*** a/src/backend/commands/async.c
--- b/src/backend/commands/async.c
***
*** 371,376 static bool asyncQueueIsFull(void);
--- 371,377
static bool asyncQueueAdvance(volatile QueuePosition *position, int entryLength);
static void asyncQueueNotificationToEntry(Notification *n, AsyncQueueEntry *qe);
static ListCell *asyncQueueAddEntries(ListCell *nextNotify);
+ static double asyncQueueSaturation(void);
static void asyncQueueFillWarning(void);
static bool SignalBackends(void);
static void asyncQueueReadAllNotifications(void);
***
*** 1362,1387 asyncQueueAddEntries(ListCell *nextNotify)
}
/*
! * Check whether the queue is at least half full, and emit a warning if so.
! *
! * This is unlikely given the size of the queue, but possible.
! * The warnings show up at most once every QUEUE_FULL_WARN_INTERVAL.
*
! * Caller must hold exclusive AsyncQueueLock.
*/
! static void
! asyncQueueFillWarning(void)
{
! int headPage = QUEUE_POS_PAGE(QUEUE_HEAD);
! int tailPage = QUEUE_POS_PAGE(QUEUE_TAIL);
! int occupied;
! double fillDegree;
! TimestampTz t;
occupied = headPage - tailPage;
if (occupied == 0)
! return; /* fast exit for common case */
if (occupied 0)
{
--- 1363,1399
}
Re: [HACKERS] [PATCH] Function to get size of asynchronous notification queue
I don't see this in the CF app; can you please add it there? Best regards, On Wed, Jun 17, 2015 at 3:31 AM, Brendan Jurd dire...@gmail.com wrote: Hello hackers, I present a patch to add a new built-in function pg_notify_queue_saturation(). The purpose of the function is to allow users to monitor the health of their notification queue. In certain cases, a client connection listening for notifications might get stuck inside a transaction, and this would cause the queue to keep filling up, until finally it reaches capacity and further attempts to NOTIFY error out. The current documentation under LISTEN explains this possible gotcha, but doesn't really suggest a useful way to address it, except to mention that warnings will show up in the log once you get to 50% saturation of the queue. Unless you happen to be eyeballing the logs when it happens, that's not a huge help. The choice of 50% as a threshold is also very much arbitrary, and by the time you hit 50% the problem has likely been going on for quite a while. If you want your nagios (or whatever) to say, alert you when the queue goes over 5% or 1%, your options are limited and awkward. The patch has almost no new code. It makes use of the existing logic for the 50% warning. I simply refactored that logic into a separate function asyncQueueSaturation, and then added pg_notify_queue_saturation to make that available in SQL. I am not convinced that pg_notify_queue_saturation is the best possible name for this function, and am very much open to other suggestions. The patch includes documentation, a regression test and an isolation test. Cheers, BJ -- 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/
