Re: [HACKERS] proposal: make NOTIFY list de-duplication optional

[David Steele]

On 3/11/16 1:46 PM, David Steele wrote:

Hi Filip,

On 2/20/16 8:00 AM, Filip Rembiałkowski wrote:

On Fri, Feb 19, 2016 at 10:09 PM, Catalin Iacob >
 wrote:
 > FWIW, I think it would be a good thing if the NOTIFY statement syntax 
were
 > not remarkably different from the syntax used in the pg_notify() function
 > call.  To do otherwise would certainly be confusing.  So on the whole
 > I'd go with the "NOTIFY channel [ , payload [ , mode ] ]" option.

 Filip, do you agree with Tom's proposal? Do you plan to rework the
 patch on these lines? If you are I'll try to review it, if not I could
 give it a shot as I'm interested in having this in 9.6.

I see that Tom's remarks give more flexibility, and your refinement
makes sense.


It looks like we are waiting on a new patch from you before this can be
reviewed.  Are you close to having that done?

Meanwhile, I have marked it "Waiting on Author".


Since there has been no activity on this thread since before the CF and 
no response from the author I have marked this "returned with feedback". 
Please feel free to resubmit for 9.7!


--
-David
da...@pgmasters.net


--
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] proposal: make NOTIFY list de-duplication optional

[David Steele]

Hi Filip,

On 2/20/16 8:00 AM, Filip Rembiałkowski wrote:
> On Fri, Feb 19, 2016 at 10:09 PM, Catalin Iacob  On 2/9/16, Tom Lane >
> wrote:
> > FWIW, I think it would be a good thing if the NOTIFY statement syntax 
> were
> > not remarkably different from the syntax used in the pg_notify() 
> function
> > call.  To do otherwise would certainly be confusing.  So on the whole
> > I'd go with the "NOTIFY channel [ , payload [ , mode ] ]" option.
> 
> Filip, do you agree with Tom's proposal? Do you plan to rework the
> patch on these lines? If you are I'll try to review it, if not I could
> give it a shot as I'm interested in having this in 9.6.
> 
> I see that Tom's remarks give more flexibility, and your refinement
> makes sense.

It looks like we are waiting on a new patch from you before this can be
reviewed.  Are you close to having that done?

Meanwhile, I have marked it "Waiting on Author".

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] proposal: make NOTIFY list de-duplication optional

[Catalin Iacob]

On Sat, Feb 20, 2016 at 2:00 PM, Filip Rembiałkowski
 wrote:
> I was stuck because both syntaxes have their ugliness. NOTIFY allows the
> payload to be NULL:
> NOTIFY chan01;
>
> How would this look like in "never" mode?
> NOTIFY chan01, NULL, 'never'; -- seems very cryptic.

The docs say:
"The information passed to the client for a notification event
includes the notification channel name, the notifying session's server
process PID, and the payload string, which is an empty string if it
has not been specified."

So a missing payload is not a SQL NULL but an empty string. This means
you would have:
NOTIFY chan01;
NOTIFY chan01, ''; -- same as above
NOTIFY chan01, '', 'maybe'; -- same as above
NOTIFY chan01, '', 'never'; -- send this all the time

Seems ok to me.


-- 
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] proposal: make NOTIFY list de-duplication optional

[Filip Rembiałkowski]

On Fri, Feb 19, 2016 at 10:09 PM, Catalin Iacob 
wrote:

> On 2/9/16, Tom Lane  wrote:
> > FWIW, I think it would be a good thing if the NOTIFY statement syntax
> were
> > not remarkably different from the syntax used in the pg_notify() function
> > call.  To do otherwise would certainly be confusing.  So on the whole
> > I'd go with the "NOTIFY channel [ , payload [ , mode ] ]" option.
>
> I'm quite interested in getting this addressed in time for 9.6 as I'll
> be using NOTIFY extensively in a project and I agree with Craig that
> the deduplication is frustrating both because you sometimes want every
> event and because it can apparently cause O(n^2) behaviour (which I
> didn't know before this thread). If another use case for suppressing
> deduplication is needed, consider publishing events like "inserted
> tuple", "deleted tuple" from triggers and a transaction that does
> "insert, delete, insert" which the client then sees as "insert,
> delete, oops nothing else".
>
> Tom's proposal allows for more flexible modes than just the ALL and
> DISTINCT keywords and accommodates the concern that DISTINCT will lead
> to bug reports about not really being distinct due to savepoints.
>
> Android has a similar thing for push notifications to mobile devices
> which they call collapse:
> https://developers.google.com/cloud-messaging/concept-options, search
> for collapse_key.
>
> So I propose NOTIFY channel [ , payload [ , collapse_mode ] ] with
> collapse mode being:
>
> * 'never'
>   ** Filip's proposed behaviour for the ALL option
>   ** if specified, every notification is queued regardless what's in the
> queue
>
> * 'maybe'
>   ** vague word allowing for flexibility in what the server decides to do
>   ** current behaviour
>   ** improves performance for big transactions if a row trigger
> creates the same payload over and over one after the other due to the
> current optimization of checking the tail of the list
>   ** has performance problems O(n^2) for big transactions with
> different payloads
>   *** the performance problems can be addressed by a different
> patch which uses a hash table, or decides to collapse less
> aggressively (Tom's check last 100 idea), or whatever else
>   *** in the meantime the 'never' mode acts as a good workaround
>
> In the future we might support an 'always' collapse_mode which would
> really be always, including across savepoints. Or an
> 'only_inside_savepoints' which guarantees the current behaviour.
>
> Filip, do you agree with Tom's proposal? Do you plan to rework the
> patch on these lines? If you are I'll try to review it, if not I could
> give it a shot as I'm interested in having this in 9.6.
>

I see that Tom's remarks give more flexibility, and your refinement makes
sense.

I was stuck because both syntaxes have their ugliness. NOTIFY allows the
payload to be NULL:
NOTIFY chan01;

How would this look like in "never" mode?
NOTIFY chan01, NULL, 'never'; -- seems very cryptic.

Re: [HACKERS] proposal: make NOTIFY list de-duplication optional

[Catalin Iacob]

On 2/9/16, Tom Lane  wrote:
> FWIW, I think it would be a good thing if the NOTIFY statement syntax were
> not remarkably different from the syntax used in the pg_notify() function
> call.  To do otherwise would certainly be confusing.  So on the whole
> I'd go with the "NOTIFY channel [ , payload [ , mode ] ]" option.

I'm quite interested in getting this addressed in time for 9.6 as I'll
be using NOTIFY extensively in a project and I agree with Craig that
the deduplication is frustrating both because you sometimes want every
event and because it can apparently cause O(n^2) behaviour (which I
didn't know before this thread). If another use case for suppressing
deduplication is needed, consider publishing events like "inserted
tuple", "deleted tuple" from triggers and a transaction that does
"insert, delete, insert" which the client then sees as "insert,
delete, oops nothing else".

Tom's proposal allows for more flexible modes than just the ALL and
DISTINCT keywords and accommodates the concern that DISTINCT will lead
to bug reports about not really being distinct due to savepoints.

Android has a similar thing for push notifications to mobile devices
which they call collapse:
https://developers.google.com/cloud-messaging/concept-options, search
for collapse_key.

So I propose NOTIFY channel [ , payload [ , collapse_mode ] ] with
collapse mode being:

* 'never'
  ** Filip's proposed behaviour for the ALL option
  ** if specified, every notification is queued regardless what's in the queue

* 'maybe'
  ** vague word allowing for flexibility in what the server decides to do
  ** current behaviour
  ** improves performance for big transactions if a row trigger
creates the same payload over and over one after the other due to the
current optimization of checking the tail of the list
  ** has performance problems O(n^2) for big transactions with
different payloads
  *** the performance problems can be addressed by a different
patch which uses a hash table, or decides to collapse less
aggressively (Tom's check last 100 idea), or whatever else
  *** in the meantime the 'never' mode acts as a good workaround

In the future we might support an 'always' collapse_mode which would
really be always, including across savepoints. Or an
'only_inside_savepoints' which guarantees the current behaviour.

Filip, do you agree with Tom's proposal? Do you plan to rework the
patch on these lines? If you are I'll try to review it, if not I could
give it a shot as I'm interested in having this in 9.6.


-- 
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] proposal: make NOTIFY list de-duplication optional

[Filip Rembiałkowski]

Another update - separated new internal function to satisfy opr_sanity.sql
diff --git a/contrib/tcn/tcn.c b/contrib/tcn/tcn.c
index 7352b29..3a6d4f5 100644
--- a/contrib/tcn/tcn.c
+++ b/contrib/tcn/tcn.c
@@ -160,7 +160,7 @@ triggered_change_notification(PG_FUNCTION_ARGS)
 	strcpy_quoted(payload, SPI_getvalue(trigtuple, tupdesc, colno), '\'');
 }
 
-Async_Notify(channel, payload->data);
+Async_Notify(channel, payload->data, false);
 			}
 			ReleaseSysCache(indexTuple);
 			break;
diff --git a/doc/src/sgml/ref/notify.sgml b/doc/src/sgml/ref/notify.sgml
index 4dd5608..933c76c 100644
--- a/doc/src/sgml/ref/notify.sgml
+++ b/doc/src/sgml/ref/notify.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  
 
-NOTIFY channel [ , payload ]
+NOTIFY [ ALL | DISTINCT ] channel [ , payload ]
 
  
 
@@ -105,6 +105,10 @@ NOTIFY channel [ , ALL is specified (contrary to DISTINCT, the
+   default), the server will deliver all notifications, including duplicates.
+   Removal of duplicate notifications takes place within transaction block,
+   finished with COMMIT, END or SAVEPOINT.
   
 
   
@@ -190,6 +194,12 @@ NOTIFY channel [ , NOTIFY command if you need to work with
 non-constant channel names and payloads.

+   
+There is a three-argument version, pg_notify(text,
+text, boolean). The third argument acts like
+the ALL keyword when set to true, and
+DISTINCT when set to false. 
+   
   
  
 
@@ -210,6 +220,21 @@ Asynchronous notification "virtual" with payload "This is the payload" received
 LISTEN foo;
 SELECT pg_notify('fo' || 'o', 'pay' || 'load');
 Asynchronous notification "foo" with payload "payload" received from server process with PID 14728.
+
+/* Identical messages from same (sub-) transaction can be eliminated - unless you use the ALL keyword */
+LISTEN bar;
+BEGIN;
+NOTIFY bar, 'Coffee please';
+NOTIFY bar, 'Coffee please';
+NOTIFY bar, 'Milk please';
+NOTIFY ALL bar, 'Milk please';
+SAVEPOINT s;
+NOTIFY bar, 'Coffee please';
+COMMIT;
+Asynchronous notification "bar" with payload "Coffee please" received from server process with PID 31517.
+Asynchronous notification "bar" with payload "Milk please" received from server process with PID 31517.
+Asynchronous notification "bar" with payload "Milk please" received from server process with PID 31517.
+Asynchronous notification "bar" with payload "Coffee please" received from server process with PID 31517.
 
  
 
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index c39ac3a..54d1680 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -524,7 +524,42 @@ pg_notify(PG_FUNCTION_ARGS)
 	/* For NOTIFY as a statement, this is checked in ProcessUtility */
 	PreventCommandDuringRecovery("NOTIFY");
 
-	Async_Notify(channel, payload);
+	Async_Notify(channel, payload, false);
+
+	PG_RETURN_VOID();
+}
+
+
+/*
+ * pg_notify_3args
+ *SQL function to send a notification event, 3-argument version
+ */
+Datum
+pg_notify_3args(PG_FUNCTION_ARGS)
+{
+	const char *channel;
+	const char *payload;
+	bool use_all;
+
+	if (PG_ARGISNULL(0))
+		channel = "";
+	else
+		channel = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+	if (PG_ARGISNULL(1))
+		payload = "";
+	else
+		payload = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+	if (PG_ARGISNULL(2))
+		use_all = false;
+	else
+		use_all = PG_GETARG_BOOL(2);
+
+	/* For NOTIFY as a statement, this is checked in ProcessUtility */
+	PreventCommandDuringRecovery("NOTIFY");
+
+	Async_Notify(channel, payload, use_all);
 
 	PG_RETURN_VOID();
 }
@@ -540,7 +575,7 @@ pg_notify(PG_FUNCTION_ARGS)
  *		^^
  */
 void
-Async_Notify(const char *channel, const char *payload)
+Async_Notify(const char *channel, const char *payload, bool use_all)
 {
 	Notification *n;
 	MemoryContext oldcontext;
@@ -570,9 +605,10 @@ Async_Notify(const char *channel, const char *payload)
 	 errmsg("payload string too long")));
 	}
 
-	/* no point in making duplicate entries in the list ... */
-	if (AsyncExistsPendingNotify(channel, payload))
-		return;
+	if (!use_all)
+		/* remove duplicate entries in the list */
+		if (AsyncExistsPendingNotify(channel, payload))
+			return;
 
 	/*
 	 * The notification list needs to live until end of transaction, so store
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index b307b48..7203f4a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -8528,11 +8528,12 @@ DropRuleStmt:
  *
  */
 
-NotifyStmt: NOTIFY ColId notify_payload
+NotifyStmt: NOTIFY all_or_distinct ColId notify_payload
 {
 	NotifyStmt *n = makeNode(NotifyStmt);
-	n->conditionname = $2;
-	n->payload = $3;
+	n->use_all = $2;
+	n->conditionname = $3;
+	n->payload = $4;
 	$$ = (Node *)n;
 }
 		;
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 045f7f0..0e50561 100644
--- 

Re: [HACKERS] proposal: make NOTIFY list de-duplication optional

[Filip Rembiałkowski]

Small update. I had to add one thing in /contrib/tcn/.
diff --git a/contrib/tcn/tcn.c b/contrib/tcn/tcn.c
index 7352b29..3a6d4f5 100644
--- a/contrib/tcn/tcn.c
+++ b/contrib/tcn/tcn.c
@@ -160,7 +160,7 @@ triggered_change_notification(PG_FUNCTION_ARGS)
 	strcpy_quoted(payload, SPI_getvalue(trigtuple, tupdesc, colno), '\'');
 }
 
-Async_Notify(channel, payload->data);
+Async_Notify(channel, payload->data, false);
 			}
 			ReleaseSysCache(indexTuple);
 			break;
diff --git a/doc/src/sgml/ref/notify.sgml b/doc/src/sgml/ref/notify.sgml
index 4dd5608..933c76c 100644
--- a/doc/src/sgml/ref/notify.sgml
+++ b/doc/src/sgml/ref/notify.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  
 
-NOTIFY channel [ , payload ]
+NOTIFY [ ALL | DISTINCT ] channel [ , payload ]
 
  
 
@@ -105,6 +105,10 @@ NOTIFY channel [ , ALL is specified (contrary to DISTINCT, the
+   default), the server will deliver all notifications, including duplicates.
+   Removal of duplicate notifications takes place within transaction block,
+   finished with COMMIT, END or SAVEPOINT.
   
 
   
@@ -190,6 +194,12 @@ NOTIFY channel [ , NOTIFY command if you need to work with
 non-constant channel names and payloads.

+   
+There is a three-argument version, pg_notify(text,
+text, boolean). The third argument acts like
+the ALL keyword when set to true, and
+DISTINCT when set to false. 
+   
   
  
 
@@ -210,6 +220,21 @@ Asynchronous notification "virtual" with payload "This is the payload" received
 LISTEN foo;
 SELECT pg_notify('fo' || 'o', 'pay' || 'load');
 Asynchronous notification "foo" with payload "payload" received from server process with PID 14728.
+
+/* Identical messages from same (sub-) transaction can be eliminated - unless you use the ALL keyword */
+LISTEN bar;
+BEGIN;
+NOTIFY bar, 'Coffee please';
+NOTIFY bar, 'Coffee please';
+NOTIFY bar, 'Milk please';
+NOTIFY ALL bar, 'Milk please';
+SAVEPOINT s;
+NOTIFY bar, 'Coffee please';
+COMMIT;
+Asynchronous notification "bar" with payload "Coffee please" received from server process with PID 31517.
+Asynchronous notification "bar" with payload "Milk please" received from server process with PID 31517.
+Asynchronous notification "bar" with payload "Milk please" received from server process with PID 31517.
+Asynchronous notification "bar" with payload "Coffee please" received from server process with PID 31517.
 
  
 
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index c39ac3a..38a8246 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -510,6 +510,7 @@ pg_notify(PG_FUNCTION_ARGS)
 {
 	const char *channel;
 	const char *payload;
+	bool use_all;
 
 	if (PG_ARGISNULL(0))
 		channel = "";
@@ -521,10 +522,15 @@ pg_notify(PG_FUNCTION_ARGS)
 	else
 		payload = text_to_cstring(PG_GETARG_TEXT_PP(1));
 
+	if (PG_NARGS() > 2 && ! PG_ARGISNULL(2))
+		use_all = PG_GETARG_BOOL(2);
+	else
+		use_all = false;
+
 	/* For NOTIFY as a statement, this is checked in ProcessUtility */
 	PreventCommandDuringRecovery("NOTIFY");
 
-	Async_Notify(channel, payload);
+	Async_Notify(channel, payload, use_all);
 
 	PG_RETURN_VOID();
 }
@@ -540,7 +546,7 @@ pg_notify(PG_FUNCTION_ARGS)
  *		^^
  */
 void
-Async_Notify(const char *channel, const char *payload)
+Async_Notify(const char *channel, const char *payload, bool use_all)
 {
 	Notification *n;
 	MemoryContext oldcontext;
@@ -570,9 +576,10 @@ Async_Notify(const char *channel, const char *payload)
 	 errmsg("payload string too long")));
 	}
 
-	/* no point in making duplicate entries in the list ... */
-	if (AsyncExistsPendingNotify(channel, payload))
-		return;
+	if (!use_all)
+		/* remove duplicate entries in the list */
+		if (AsyncExistsPendingNotify(channel, payload))
+			return;
 
 	/*
 	 * The notification list needs to live until end of transaction, so store
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index b307b48..7203f4a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -8528,11 +8528,12 @@ DropRuleStmt:
  *
  */
 
-NotifyStmt: NOTIFY ColId notify_payload
+NotifyStmt: NOTIFY all_or_distinct ColId notify_payload
 {
 	NotifyStmt *n = makeNode(NotifyStmt);
-	n->conditionname = $2;
-	n->payload = $3;
+	n->use_all = $2;
+	n->conditionname = $3;
+	n->payload = $4;
 	$$ = (Node *)n;
 }
 		;
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 045f7f0..0e50561 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -599,7 +599,7 @@ standard_ProcessUtility(Node *parsetree,
 NotifyStmt *stmt = (NotifyStmt *) parsetree;
 
 PreventCommandDuringRecovery("NOTIFY");
-Async_Notify(stmt->conditionname, stmt->payload);
+Async_Notify(stmt->conditionname, stmt->payload, stmt->use_all);
 			}
 			break;
 
diff --git 

Re: [HACKERS] proposal: make NOTIFY list de-duplication optional

[Josh Kupershmidt]

On Tue, Feb 9, 2016 at 2:16 PM, Filip Rembiałkowski
 wrote:
> But then it becomes disputable if SQL syntax change makes sense.
>
> ---we had this,
>  NOTIFY channel [ , payload ]
> ---and in this patch we have this
> NOTIFY [ ALL | DISTINCT ] channel [ , payload ]
>  ---  but maybe we should have this?
> NOTIFY channel [ , payload [ , mode ] ]

I think using ALL to mean "don't worry about de-duplication" could be
a bit confusing, especially as there was some interest recently in
supporting wildcard notifications:
http://www.postgresql.org/message-id/52693fc5.7070...@gmail.com

and conceivably we might want to support a way to notify all
listeners, i.e. NOTIFY * as proposed in that thread. If we ever
supported wildcard notifies, ALL may be easily confused to mean "all
channel names".

What about adopting the options-inside-parentheses format, the way
EXPLAIN does nowadays, something like:

NOTIFY (DEDUPLICATE FALSE, MODE IMMEDIATE) mychannel;

Josh


-- 
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] proposal: make NOTIFY list de-duplication optional

[Tom Lane]

Josh Kupershmidt  writes:
> On Tue, Feb 9, 2016 at 2:16 PM, Filip Rembiałkowski
>  wrote:
>> But then it becomes disputable if SQL syntax change makes sense.
>> 
>> ---we had this,
>> NOTIFY channel [ , payload ]
>> ---and in this patch we have this
>> NOTIFY [ ALL | DISTINCT ] channel [ , payload ]
>> ---  but maybe we should have this?
>> NOTIFY channel [ , payload [ , mode ] ]

> What about adopting the options-inside-parentheses format, the way
> EXPLAIN does nowadays, something like:
> NOTIFY (DEDUPLICATE FALSE, MODE IMMEDIATE) mychannel;

FWIW, I think it would be a good thing if the NOTIFY statement syntax were
not remarkably different from the syntax used in the pg_notify() function
call.  To do otherwise would certainly be confusing.  So on the whole
I'd go with the "NOTIFY channel [ , payload [ , mode ] ]" option.

regards, tom lane


-- 
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] proposal: make NOTIFY list de-duplication optional

[Filip Rembiałkowski]

On Tue, Feb 9, 2016 at 12:15 AM, Merlin Moncure  wrote:

> I wonder if the third argument
> should be a boolean however.  If we make it 'text, 'send mode',
> instead, we could leave some room for more specialization of the
> queuing behavior.
>
> For example, we've had a couple of requests over the years to have an
> 'immediate' mode which dumps the notification immediately to the
> client without waiting for tx commit. This may or may not be a good
> idea, but if it was ultimately proved to be, it could be introduced as
> an alternate mode without adding an extra function.

But then it becomes disputable if SQL syntax change makes sense.

---we had this,
 NOTIFY channel [ , payload ]
---and in this patch we have this
NOTIFY [ ALL | DISTINCT ] channel [ , payload ]
 ---  but maybe we should have this?
NOTIFY channel [ , payload [ , mode ] ]

I'm not sure which direction is better with non-standard SQL additions.
Recycling keywords or adding more commas?


-- 
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] proposal: make NOTIFY list de-duplication optional

[Craig Ringer]

On 8 February 2016 at 09:37, Filip Rembiałkowski <
filip.rembialkow...@gmail.com> wrote:

> On Sun, Feb 7, 2016 at 4:37 PM, Vik Fearing  wrote:
>
> >>> There is also no mention in the documentation about what happens if I
> do:
> >>>
> >>> NOTIFY ALL chan, 'msg';
> >>> NOTIFY ALL chan, 'msg';
> >>> NOTIFY DISTINCT chan, 'msg';
> >>> NOTIFY ALL chan, 'msg';
> >>>
> >>> Without testing, I'd say I'd get two messages, but it should be
> >>> explicitly mentioned somewhere.
> >>
> >> If it's four separate transactions, LISTEN'er should get four.
> >
> > The question was for one transaction, I should have been clearer about
> that.
> >
> >> If it's in one transaction,  LISTEN'er should get three.
> >
> > This is surprising to me, I would think it would get only two.  What is
> > your rationale for three?
> >
>
> It is a single transaction, but four separate commands.
>
> >>> NOTIFY ALL chan, 'msg';
> -- send the message, save in the list/hash
> >>> NOTIFY ALL chan, 'msg';
> -- ALL was specified, send the message even if it is on the list/hash
> >>> NOTIFY DISTINCT chan, 'msg';
> -- default mode, skip message because it's in the list/hash
> >>> NOTIFY ALL chan, 'msg';
> -- ALL was specified, send the message even if it is hashed/saved
>

So in total three messages are sent?

Would it be correct to say that if ALL is specified then a message is
queued no matter what. If DISTINCT is specified then it is only queued if
no message with the same channel and argument is already queued for
delivery. Using DISTINCT can never decrease the total number of messages to
be sent.

Right?

If so, I think that's the right behaviour and the docs just need to be
explicit - an example like the above would be good, translated to be
friendlier to those who don't know the internal mechanics.

I've found the deduplication functionality of NOTIFY very frustrating in
the past and I see this as a significant improvement. Sometimes the *number
of times* something happened is significant too...

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] proposal: make NOTIFY list de-duplication optional

[Vik Fearing]

On 02/08/2016 09:33 PM, Filip Rembiałkowski wrote:
> Here is my next try, after suggestions from -perf and -hackers list:
> 
> * no GUC
> 
> * small addition to NOTIFY grammar: NOTIFY ALL/DISTINCT
> 
> * corresponding, 3-argument version of pg_notify(text,text,bool)
> 
> * updated the docs to include new syntax and clarify behavior
> 
> * no hashtable in AsyncExistsPendingNotify
>  (I don't see much sense in that part; and it can be well done
> separately from this)

Please add this to the next commitfest:
https://commitfest.postgresql.org/9/new/
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
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] proposal: make NOTIFY list de-duplication optional

[Filip Rembiałkowski]

On Mon, Feb 8, 2016 at 1:52 PM, Craig Ringer  wrote:

> Would it be correct to say that if ALL is specified then a message is queued
> no matter what. If DISTINCT is specified then it is only queued if no
> message with the same channel and argument is already queued for delivery.
Yes, exactly.

> Using DISTINCT can never decrease the total number of messages to be sent.
This sentence does not sound true. DISTINCT is the default, old
behaviour. It *can* decrease total number of messages (by
deduplication)

> I've found the deduplication functionality of NOTIFY very frustrating in the 
> past
> and I see this as a significant improvement. Sometimes the *number of times*
> something happened is significant too...
yep, same idea here.




Here is my next try, after suggestions from -perf and -hackers list:

* no GUC

* small addition to NOTIFY grammar: NOTIFY ALL/DISTINCT

* corresponding, 3-argument version of pg_notify(text,text,bool)

* updated the docs to include new syntax and clarify behavior

* no hashtable in AsyncExistsPendingNotify
 (I don't see much sense in that part; and it can be well done
separately from this)
diff --git a/doc/src/sgml/ref/notify.sgml b/doc/src/sgml/ref/notify.sgml
index 4dd5608..933c76c 100644
--- a/doc/src/sgml/ref/notify.sgml
+++ b/doc/src/sgml/ref/notify.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  
 
-NOTIFY channel [ , payload ]
+NOTIFY [ ALL | DISTINCT ] channel [ , payload ]
 
  
 
@@ -105,6 +105,10 @@ NOTIFY channel [ , ALL is specified (contrary to DISTINCT, the
+   default), the server will deliver all notifications, including duplicates.
+   Removal of duplicate notifications takes place within transaction block,
+   finished with COMMIT, END or SAVEPOINT.
   
 
   
@@ -190,6 +194,12 @@ NOTIFY channel [ , NOTIFY command if you need to work with
 non-constant channel names and payloads.

+   
+There is a three-argument version, pg_notify(text,
+text, boolean). The third argument acts like
+the ALL keyword when set to true, and
+DISTINCT when set to false. 
+   
   
  
 
@@ -210,6 +220,21 @@ Asynchronous notification "virtual" with payload "This is the payload" received
 LISTEN foo;
 SELECT pg_notify('fo' || 'o', 'pay' || 'load');
 Asynchronous notification "foo" with payload "payload" received from server process with PID 14728.
+
+/* Identical messages from same (sub-) transaction can be eliminated - unless you use the ALL keyword */
+LISTEN bar;
+BEGIN;
+NOTIFY bar, 'Coffee please';
+NOTIFY bar, 'Coffee please';
+NOTIFY bar, 'Milk please';
+NOTIFY ALL bar, 'Milk please';
+SAVEPOINT s;
+NOTIFY bar, 'Coffee please';
+COMMIT;
+Asynchronous notification "bar" with payload "Coffee please" received from server process with PID 31517.
+Asynchronous notification "bar" with payload "Milk please" received from server process with PID 31517.
+Asynchronous notification "bar" with payload "Milk please" received from server process with PID 31517.
+Asynchronous notification "bar" with payload "Coffee please" received from server process with PID 31517.
 
  
 
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index c39ac3a..38a8246 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -510,6 +510,7 @@ pg_notify(PG_FUNCTION_ARGS)
 {
 	const char *channel;
 	const char *payload;
+	bool use_all;
 
 	if (PG_ARGISNULL(0))
 		channel = "";
@@ -521,10 +522,15 @@ pg_notify(PG_FUNCTION_ARGS)
 	else
 		payload = text_to_cstring(PG_GETARG_TEXT_PP(1));
 
+	if (PG_NARGS() > 2 && ! PG_ARGISNULL(2))
+		use_all = PG_GETARG_BOOL(2);
+	else
+		use_all = false;
+
 	/* For NOTIFY as a statement, this is checked in ProcessUtility */
 	PreventCommandDuringRecovery("NOTIFY");
 
-	Async_Notify(channel, payload);
+	Async_Notify(channel, payload, use_all);
 
 	PG_RETURN_VOID();
 }
@@ -540,7 +546,7 @@ pg_notify(PG_FUNCTION_ARGS)
  *		^^
  */
 void
-Async_Notify(const char *channel, const char *payload)
+Async_Notify(const char *channel, const char *payload, bool use_all)
 {
 	Notification *n;
 	MemoryContext oldcontext;
@@ -570,9 +576,10 @@ Async_Notify(const char *channel, const char *payload)
 	 errmsg("payload string too long")));
 	}
 
-	/* no point in making duplicate entries in the list ... */
-	if (AsyncExistsPendingNotify(channel, payload))
-		return;
+	if (!use_all)
+		/* remove duplicate entries in the list */
+		if (AsyncExistsPendingNotify(channel, payload))
+			return;
 
 	/*
 	 * The notification list needs to live until end of transaction, so store
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index b307b48..7203f4a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -8528,11 +8528,12 @@ DropRuleStmt:
  *
  */
 
-NotifyStmt: NOTIFY ColId notify_payload
+NotifyStmt: NOTIFY all_or_distinct ColId notify_payload
 {
 		

Re: [HACKERS] proposal: make NOTIFY list de-duplication optional

[Merlin Moncure]

On Mon, Feb 8, 2016 at 2:33 PM, Filip Rembiałkowski
 wrote:
> Here is my next try, after suggestions from -perf and -hackers list:
>
> * no GUC
>
> * small addition to NOTIFY grammar: NOTIFY ALL/DISTINCT
>
> * corresponding, 3-argument version of pg_notify(text,text,bool)

This is all sounding pretty good.   I wonder if the third argument
should be a boolean however.  If we make it 'text, 'send mode',
instead, we could leave some room for more specialization of the
queuing behavior.

For example, we've had a couple of requests over the years to have an
'immediate' mode which dumps the notification immediately to the
client without waiting for tx commit. This may or may not be a good
idea, but if it was ultimately proved to be, it could be introduced as
an alternate mode without adding an extra function.

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] proposal: make NOTIFY list de-duplication optional

[Filip Rembiałkowski]

On Sun, Feb 7, 2016 at 11:54 AM, Vik Fearing  wrote:
> On 02/07/2016 03:42 AM, Filip Rembiałkowski wrote:

> You left the duplicate behavior with subtransactions, but didn't mention
> it in the documentation.  If I do  NOTIFY DISTINCT chan, 'msg';  then I
> expect only distinct notifications but I'll get duplicates if I'm in a
> subtransaction.  Either the documentation or the code needs to be fixed.

agreed

>
> I seem to remember some discussion about not using DEFAULT parameters in
> system functions so you should leave the old function alone and create a
> new function with your use_all parameter.  I don't recall the exact
> reason why so hopefully someone else will enlighten me.

I'm quite new to this; how do I pinpoint proper OID for a new catalog
object (function, in this case)?
Is there a better way than browsing fmgr files and guessing next available oid?

>
> There is also no mention in the documentation about what happens if I do:
>
> NOTIFY ALL chan, 'msg';
> NOTIFY ALL chan, 'msg';
> NOTIFY DISTINCT chan, 'msg';
> NOTIFY ALL chan, 'msg';
>
> Without testing, I'd say I'd get two messages, but it should be
> explicitly mentioned somewhere.

If it's four separate transactions, LISTEN'er should get four.
If it's in one transaction,  LISTEN'er should get three.










> --
> Vik Fearing  +33 6 46 75 15 36
> http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
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] proposal: make NOTIFY list de-duplication optional

[Vik Fearing]

On 02/07/2016 04:00 PM, Filip Rembiałkowski wrote:
> On Sun, Feb 7, 2016 at 11:54 AM, Vik Fearing  wrote:
>> I seem to remember some discussion about not using DEFAULT parameters in
>> system functions so you should leave the old function alone and create a
>> new function with your use_all parameter.  I don't recall the exact
>> reason why so hopefully someone else will enlighten me.
> 
> I'm quite new to this; how do I pinpoint proper OID for a new catalog
> object (function, in this case)?
> Is there a better way than browsing fmgr files and guessing next available 
> oid?

There is a shell script called `unused_oids` in src/include/catalog/.

>> There is also no mention in the documentation about what happens if I do:
>>
>> NOTIFY ALL chan, 'msg';
>> NOTIFY ALL chan, 'msg';
>> NOTIFY DISTINCT chan, 'msg';
>> NOTIFY ALL chan, 'msg';
>>
>> Without testing, I'd say I'd get two messages, but it should be
>> explicitly mentioned somewhere.
> 
> If it's four separate transactions, LISTEN'er should get four.

The question was for one transaction, I should have been clearer about that.

> If it's in one transaction,  LISTEN'er should get three.

This is surprising to me, I would think it would get only two.  What is
your rationale for three?

Compare with the behavior of:

select 1
union all
select 1
union distinct
select 1
union all
select 1;
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
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] proposal: make NOTIFY list de-duplication optional

[Vik Fearing]

On 02/07/2016 03:42 AM, Filip Rembiałkowski wrote:
> +1
> 
> ... and a patch (only adding ALL keyword, no hash table implemented yet).

Please stop top-posting, it's very disruptive.  My comments are below,
where they belong.

> On Sat, Feb 6, 2016 at 2:35 PM, Brendan Jurd  wrote:
>> On Sat, 6 Feb 2016 at 12:50 Tom Lane  wrote:
>>>
>>> Robert Haas  writes:
 I agree with what Merlin said about this:

 http://www.postgresql.org/message-id/CAHyXU0yoHe8Qc=yc10ahu1nfia1tbhsg+35ds-oeueuapo7...@mail.gmail.com
>>>
>>> Yeah, I agree that a GUC for this is quite unappetizing.
>>
>>
>> How would you feel about a variant for calling NOTIFY?
>>
>> The SQL syntax could be something like "NOTIFY [ALL] channel, payload" where
>> the ALL means "just send the notification already, nobody cares whether
>> there's an identical one in the queue".
>>
>> Likewise we could introduce a three-argument form of pg_notify(text, text,
>> bool) where the final argument is whether you are interested in removing
>> duplicates.
>>
>> Optimising the remove-duplicates path is still probably a worthwhile
>> endeavour, but if the user really doesn't care at all about duplication, it
>> seems silly to force them to pay any performance price for a behaviour they
>> didn't want, no?

On 02/07/2016 03:42 AM, Filip Rembiałkowski wrote:
> +1
>
> ... and a patch (only adding ALL keyword, no hash table implemented yet).


I only read through the patch, I didn't compile it or test it, but I
have a few comments:

You left the duplicate behavior with subtransactions, but didn't mention
it in the documentation.  If I do  NOTIFY DISTINCT chan, 'msg';  then I
expect only distinct notifications but I'll get duplicates if I'm in a
subtransaction.  Either the documentation or the code needs to be fixed.

I seem to remember some discussion about not using DEFAULT parameters in
system functions so you should leave the old function alone and create a
new function with your use_all parameter.  I don't recall the exact
reason why so hopefully someone else will enlighten me.

There is also no mention in the documentation about what happens if I do:

NOTIFY ALL chan, 'msg';
NOTIFY ALL chan, 'msg';
NOTIFY DISTINCT chan, 'msg';
NOTIFY ALL chan, 'msg';

Without testing, I'd say I'd get two messages, but it should be
explicitly mentioned somewhere.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


-- 
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] proposal: make NOTIFY list de-duplication optional

[Filip Rembiałkowski]

On Sun, Feb 7, 2016 at 4:37 PM, Vik Fearing  wrote:

>>> There is also no mention in the documentation about what happens if I do:
>>>
>>> NOTIFY ALL chan, 'msg';
>>> NOTIFY ALL chan, 'msg';
>>> NOTIFY DISTINCT chan, 'msg';
>>> NOTIFY ALL chan, 'msg';
>>>
>>> Without testing, I'd say I'd get two messages, but it should be
>>> explicitly mentioned somewhere.
>>
>> If it's four separate transactions, LISTEN'er should get four.
>
> The question was for one transaction, I should have been clearer about that.
>
>> If it's in one transaction,  LISTEN'er should get three.
>
> This is surprising to me, I would think it would get only two.  What is
> your rationale for three?
>

It is a single transaction, but four separate commands.

>>> NOTIFY ALL chan, 'msg';
-- send the message, save in the list/hash
>>> NOTIFY ALL chan, 'msg';
-- ALL was specified, send the message even if it is on the list/hash
>>> NOTIFY DISTINCT chan, 'msg';
-- default mode, skip message because it's in the list/hash
>>> NOTIFY ALL chan, 'msg';
-- ALL was specified, send the message even if it is hashed/saved


-- 
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] proposal: make NOTIFY list de-duplication optional

[Tom Lane]

Brendan Jurd  writes:
> On Sat, 6 Feb 2016 at 12:50 Tom Lane  wrote:
>> Yeah, I agree that a GUC for this is quite unappetizing.

> How would you feel about a variant for calling NOTIFY?

If we decide that this ought to be user-visible, then an extra NOTIFY
parameter would be the way to do it.  I'd much rather it "just works"
though.  In particular, if we do start advertising user control of
de-duplication, we are likely to start getting bug reports about every
case where it's inexact, eg the no-checks-across-subxact-boundaries
business.

> Optimising the remove-duplicates path is still probably a worthwhile
> endeavour, but if the user really doesn't care at all about duplication, it
> seems silly to force them to pay any performance price for a behaviour they
> didn't want, no?

I would only be impressed with that argument if it could be shown that
de-duplication was a significant fraction of the total cost of a typical
NOTIFY cycle.  Obviously, you can make the O(N^2) term dominate if you
try, but I really doubt that it's significant for reasonable numbers of
notify events per transaction.  One should also keep in mind that
duplicate events are going to cost extra processing on the
client-application side, too.  In my experience with using NOTIFY, that
cost probably dwarfs the cost of emitting the messages.

regards, tom lane


-- 
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] proposal: make NOTIFY list de-duplication optional

[Andrew Dunstan]

On 02/05/2016 08:49 PM, Tom Lane wrote:


Yeah, I agree that a GUC for this is quite unappetizing.


Agreed.



One idea would be to build a hashtable to aid with duplicate detection
(perhaps only once the pending-notify list gets long).

Another thought is that it's already the case that duplicate detection is
something of a "best effort" activity; note for example the comment in
AsyncExistsPendingNotify pointing out that we don't collapse duplicates
across subtransactions.  Would it be acceptable to relax the standards
a bit further?  For example, if we only checked for duplicates among the
last N notification list entries (for N say around 100), we'd probably
cover just about all the useful cases, and the runtime would stay linear.
The data structure isn't tremendously conducive to that, but it could be
done.





I like the hashtable idea if it can be made workable.

cheers

andrew


--
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] proposal: make NOTIFY list de-duplication optional

[Filip Rembiałkowski]

+1

... and a patch (only adding ALL keyword, no hash table implemented yet).



On Sat, Feb 6, 2016 at 2:35 PM, Brendan Jurd  wrote:
> On Sat, 6 Feb 2016 at 12:50 Tom Lane  wrote:
>>
>> Robert Haas  writes:
>> > I agree with what Merlin said about this:
>> >
>> > http://www.postgresql.org/message-id/CAHyXU0yoHe8Qc=yc10ahu1nfia1tbhsg+35ds-oeueuapo7...@mail.gmail.com
>>
>> Yeah, I agree that a GUC for this is quite unappetizing.
>
>
> How would you feel about a variant for calling NOTIFY?
>
> The SQL syntax could be something like "NOTIFY [ALL] channel, payload" where
> the ALL means "just send the notification already, nobody cares whether
> there's an identical one in the queue".
>
> Likewise we could introduce a three-argument form of pg_notify(text, text,
> bool) where the final argument is whether you are interested in removing
> duplicates.
>
> Optimising the remove-duplicates path is still probably a worthwhile
> endeavour, but if the user really doesn't care at all about duplication, it
> seems silly to force them to pay any performance price for a behaviour they
> didn't want, no?
>
> Cheers,
> BJ
diff --git a/doc/src/sgml/ref/notify.sgml b/doc/src/sgml/ref/notify.sgml
index 4dd5608..c148859 100644
--- a/doc/src/sgml/ref/notify.sgml
+++ b/doc/src/sgml/ref/notify.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  
 
-NOTIFY channel [ , payload ]
+NOTIFY [ ALL | DISTINCT ] channel [ , payload ]
 
  
 
@@ -105,6 +105,8 @@ NOTIFY channel [ , ALL is specified (contrary to DISTINCT, the
+   default), the server will deliver all notifications, including duplicates.
   
 
   
@@ -184,11 +186,14 @@ NOTIFY channel [ , 
 To send a notification you can also use the function
-pg_notify(text,
-text). The function takes the channel name as the
-first argument and the payload as the second. The function is much easier
-to use than the NOTIFY command if you need to work with
-non-constant channel names and payloads.
+pg_notify(channel text,
+payload text ,
+use_all boolean).
+The function takes the channel name as the first argument and the payload
+as the second. The third argument, false by default, represents
+the ALL keyword. The function is much easier to use than the
+NOTIFY command if you need to work with non-constant
+channel names and payloads.

   
  
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 923fe58..9df5301 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -965,3 +965,10 @@ RETURNS jsonb
 LANGUAGE INTERNAL
 STRICT IMMUTABLE
 AS 'jsonb_set';
+
+CREATE OR REPLACE FUNCTION
+  pg_notify(channel text, payload text, use_all boolean DEFAULT false)
+RETURNS void
+LANGUAGE INTERNAL
+VOLATILE
+AS 'pg_notify';
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index c39ac3a..d374a00 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -510,6 +510,7 @@ pg_notify(PG_FUNCTION_ARGS)
 {
 	const char *channel;
 	const char *payload;
+	bool use_all;
 
 	if (PG_ARGISNULL(0))
 		channel = "";
@@ -521,10 +522,12 @@ pg_notify(PG_FUNCTION_ARGS)
 	else
 		payload = text_to_cstring(PG_GETARG_TEXT_PP(1));
 
+	use_all = PG_GETARG_BOOL(2);
+
 	/* For NOTIFY as a statement, this is checked in ProcessUtility */
 	PreventCommandDuringRecovery("NOTIFY");
 
-	Async_Notify(channel, payload);
+	Async_Notify(channel, payload, use_all);
 
 	PG_RETURN_VOID();
 }
@@ -540,7 +543,7 @@ pg_notify(PG_FUNCTION_ARGS)
  *		^^
  */
 void
-Async_Notify(const char *channel, const char *payload)
+Async_Notify(const char *channel, const char *payload, bool use_all)
 {
 	Notification *n;
 	MemoryContext oldcontext;
@@ -570,9 +573,10 @@ Async_Notify(const char *channel, const char *payload)
 	 errmsg("payload string too long")));
 	}
 
-	/* no point in making duplicate entries in the list ... */
-	if (AsyncExistsPendingNotify(channel, payload))
-		return;
+	if (!use_all)
+		/* remove duplicate entries in the list */
+		if (AsyncExistsPendingNotify(channel, payload))
+			return;
 
 	/*
 	 * The notification list needs to live until end of transaction, so store
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index b307b48..7203f4a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -8528,11 +8528,12 @@ DropRuleStmt:
  *
  */
 
-NotifyStmt: NOTIFY ColId notify_payload
+NotifyStmt: NOTIFY all_or_distinct ColId notify_payload
 {
 	NotifyStmt *n = makeNode(NotifyStmt);
-	n->conditionname = $2;
-	n->payload = $3;
+	n->use_all = $2;
+	n->conditionname = $3;
+	n->payload = $4;
 	$$ = (Node *)n;
 }
 		;
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 045f7f0..0e50561 

Re: [HACKERS] proposal: make NOTIFY list de-duplication optional

[Brendan Jurd]

On Sat, 6 Feb 2016 at 12:50 Tom Lane  wrote:

> Robert Haas  writes:
> > I agree with what Merlin said about this:
> >
> http://www.postgresql.org/message-id/CAHyXU0yoHe8Qc=yc10ahu1nfia1tbhsg+35ds-oeueuapo7...@mail.gmail.com
>
> Yeah, I agree that a GUC for this is quite unappetizing.
>

How would you feel about a variant for calling NOTIFY?

The SQL syntax could be something like "NOTIFY [ALL] channel, payload"
where the ALL means "just send the notification already, nobody cares
whether there's an identical one in the queue".

Likewise we could introduce a three-argument form of pg_notify(text, text,
bool) where the final argument is whether you are interested in removing
duplicates.

Optimising the remove-duplicates path is still probably a worthwhile
endeavour, but if the user really doesn't care at all about duplication, it
seems silly to force them to pay any performance price for a behaviour they
didn't want, no?

Cheers,
BJ

[HACKERS] proposal: make NOTIFY list de-duplication optional

[Filip Rembiałkowski]

- new GUC in "Statement Behaviour" section, notify_duplicate_removal
(default true)

Initial discussion in this thread:
http://www.postgresql.org/message-id/CAP_rwwmpzk9=sbjrztod05bdctyc43wnknu_m37dygvl4sa...@mail.gmail.com

Rationale: for some legitimate use cases, duplicate removal is not
required, and it gets O(N^2) cost on large COPY/ insert transactions.
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 392eb70..9fb5504 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -6095,6 +6095,24 @@ SET XML OPTION { DOCUMENT | CONTENT };
   
  
 
+ 
+  notify_duplicate_removal (bool)
+  
+   notify_duplicate_removal configuration parameter
+  
+  
+  
+   
+Try to remove duplicate messages while processing NOTIFY.  When
+on (the default), the server will avoid duplicate messages
+(with same channel and payload).  Setting this variable to
+off can increase performance in some situations - at the
+cost of having duplicate messages in NOTIFY queue.  See  for more information.
+   
+  
+ 
+
  
 
  
diff --git a/doc/src/sgml/ref/notify.sgml b/doc/src/sgml/ref/notify.sgml
index 4dd5608..86a9bed 100644
--- a/doc/src/sgml/ref/notify.sgml
+++ b/doc/src/sgml/ref/notify.sgml
@@ -95,16 +95,17 @@ NOTIFY channel [ , 
If the same channel name is signaled multiple times from the same
-   transaction with identical payload strings, the
-   database server can decide to deliver a single notification only.
-   On the other hand, notifications with distinct payload strings will
-   always be delivered as distinct notifications. Similarly, notifications from
-   different transactions will never get folded into one notification.
-   Except for dropping later instances of duplicate notifications,
+   transaction with identical payload strings, and
+   notify_duplicate_removal is set to true, the database server
+   can decide to deliver a single notification only.  On the other hand,
+   notifications with distinct payload strings will always be delivered as
+   distinct notifications. Similarly, notifications from different
+   transactions will never get folded into one notification.  Except for
+   dropping later instances of duplicate notifications,
NOTIFY guarantees that notifications from the same
transaction get delivered in the order they were sent.  It is also
-   guaranteed that messages from different transactions are delivered in
-   the order in which the transactions committed.
+   guaranteed that messages from different transactions are delivered in the
+   order in which the transactions committed.
   
 
   
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index c39ac3a..a7bc9f1 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -364,8 +364,9 @@ static bool amRegisteredListener = false;
 /* has this backend sent notifications in the current transaction? */
 static bool backendHasSentNotifications = false;
 
-/* GUC parameter */
+/* GUC parameters */
 bool		Trace_notify = false;
+bool		notify_duplicate_removal = true;
 
 /* local function prototypes */
 static bool asyncQueuePagePrecedes(int p, int q);
@@ -570,9 +571,12 @@ Async_Notify(const char *channel, const char *payload)
 	 errmsg("payload string too long")));
 	}
 
-	/* no point in making duplicate entries in the list ... */
-	if (AsyncExistsPendingNotify(channel, payload))
-		return;
+	if (notify_duplicate_removal)
+	{
+		/* check for duplicate entries in the list */
+		if (AsyncExistsPendingNotify(channel, payload))
+			return;
+	}
 
 	/*
 	 * The notification list needs to live until end of transaction, so store
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 83b8388..b737c29 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1617,6 +1617,15 @@ static struct config_bool ConfigureNamesBool[] =
 		false,
 		NULL, NULL, NULL
 	},
+	{
+		{"notify_duplicate_removal", PGC_USERSET, CLIENT_CONN_STATEMENT,
+			gettext_noop("Remove duplicate messages during NOTIFY."),
+			NULL
+		},
+		_duplicate_removal,
+		true,
+		NULL, NULL, NULL
+	},
 
 	/* End-of-list marker */
 	{
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 029114f..2831c1b 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -536,6 +536,7 @@
 #xmloption = 'content'
 #gin_fuzzy_search_limit = 0
 #gin_pending_list_limit = 4MB
+#notify_duplicate_removal = on
 
 # - Locale and Formatting -
 
diff --git a/src/include/commands/async.h b/src/include/commands/async.h
index b4c13fa..c572691 100644
--- a/src/include/commands/async.h
+++ b/src/include/commands/async.h
@@ -23,6 +23,7 @@
 #define NUM_ASYNC_BUFFERS	8
 
 extern bool Trace_notify;
+extern bool notify_duplicate_removal;
 extern volatile sig_atomic_t 

Re: [HACKERS] proposal: make NOTIFY list de-duplication optional

[Tom Lane]

Robert Haas  writes:
> On Fri, Feb 5, 2016 at 10:17 AM, Filip Rembiałkowski
>  wrote:
>> - new GUC in "Statement Behaviour" section, notify_duplicate_removal

> I agree with what Merlin said about this:
> http://www.postgresql.org/message-id/CAHyXU0yoHe8Qc=yc10ahu1nfia1tbhsg+35ds-oeueuapo7...@mail.gmail.com

Yeah, I agree that a GUC for this is quite unappetizing.

One idea would be to build a hashtable to aid with duplicate detection
(perhaps only once the pending-notify list gets long).

Another thought is that it's already the case that duplicate detection is
something of a "best effort" activity; note for example the comment in
AsyncExistsPendingNotify pointing out that we don't collapse duplicates
across subtransactions.  Would it be acceptable to relax the standards
a bit further?  For example, if we only checked for duplicates among the
last N notification list entries (for N say around 100), we'd probably
cover just about all the useful cases, and the runtime would stay linear.
The data structure isn't tremendously conducive to that, but it could be
done.

regards, tom lane


-- 
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] proposal: make NOTIFY list de-duplication optional

[Robert Haas]

On Fri, Feb 5, 2016 at 10:17 AM, Filip Rembiałkowski
 wrote:
> - new GUC in "Statement Behaviour" section, notify_duplicate_removal
> (default true)
>
> Initial discussion in this thread:
> http://www.postgresql.org/message-id/CAP_rwwmpzk9=sbjrztod05bdctyc43wnknu_m37dygvl4sa...@mail.gmail.com
>
> Rationale: for some legitimate use cases, duplicate removal is not required,
> and it gets O(N^2) cost on large COPY/ insert transactions.

I agree with what Merlin said about this:

http://www.postgresql.org/message-id/CAHyXU0yoHe8Qc=yc10ahu1nfia1tbhsg+35ds-oeueuapo7...@mail.gmail.com

-- 
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