Re: [HACKERS] [PATCH] Pattern based listeners for asynchronous messaging (LISTEN/NOTIFY)

[Markus Sintonen]

Hi Marko!
Thanks for the good feedback.

Good point on the pg_listening_channels(). Do you think we could change the
interface of the function? At least PG v10 has changed functions elsewhere
quite dramatically eg. related to xlog functions.
We could change the pg_listening_channels() return type to 'setof record'
which would include name (text) and isPattern (bool). This would also allow
some future additions (eg. time of last received notification). One other
way with better backward compatibility would be to add some kind of postfix
to patterns to identify them and keep the 'text' return type. It would
return eg. 'foo% - PATTERN' for a pattern, but this is quite nasty. Second
way would be to add totally new function eg. 'pg_listening_channels_info'
with return type of 'setof record' and keep the original function as it is
(just change the behaviour on duplicated names and patterns as you showed).

You also have a good point on the UNLISTEN. At first I thought that
UNLISTEN SIMILAR TO 'xxx' would be semantically confusing since it fells
that it would do some pattern based unregistering. But by documenting the
behaviour it might be ok. I also agree that it would be best to have
distinction between unlistening patterns and regular names.

I'll reflect on the rest of the feedback on the next patch if we reach some
conclusion on the pg_listening_channels and UNLISTEN.

BR
Markus

2017-09-08 2:44 GMT+03:00 Marko Tiikkaja :

> Hi Markus,
>
> On Sun, Aug 20, 2017 at 9:56 PM, Markus Sintonen <
> markus.sinto...@gmail.com> wrote:
>
>> I also encountered this when I built it with different configuration. I
>> attached updated patch with the correct number of arguments to
>> 'similar_escape'. I also added preliminary documentation to the patch.
>> (Unfortunately unable to currently compile the documentation for testing
>> purpose on Windows probably because of commit https://github.com/post
>> gres/postgres/commit/510074f9f0131a04322d6a3d2a51c87e6db243f9. I
>> followed https://www.postgresql.org/docs/devel/static/install-windows
>> -full.html#idm45412738673840.)
>>
>> What do you think about the syntax? There was a suggestion to specify
>> type of the pattern (eg ltree extension) but to me this feels like a
>> overkill.
>> One option here would be eg:
>> LISTEN PATTERN 'foo%' TYPE 'similar'
>> LISTEN PATTERN 'foo.*' TYPE 'ltree'
>>
>
> Not that my opinion matters, but I think we should pick one pattern style
> and stick to it.  SIMILAR TO doesn't seem like the worst choice.  ltree
> seems useless.
>
> As for the rest of the interface..
>
> First, I think mixing patterns and non-patterns is weird.  This is
> apparent in at least two cases:
>
> marko=# listen "foo%";
> LISTEN
> marko=# listen similar to 'foo%';
> LISTEN
> marko=# select * from pg_listening_channels();
>  pg_listening_channels
> ---
>  foo%
> (1 row)
>
> -- Not actually listening on the pattern.  Confusion.
>
> The second case being the way UNLISTEN can be used to unlisten patterns,
> too.  It kind of makes sense given that you can't really end up with both a
> channel name and a pattern with the same source string, but it's still
> weird.  I think it would be much better to keep these completely separate
> so that you could be listening on both the channel "foo%" and the pattern
> 'foo%' at the same time, and you'd use a bare UNLISTEN to unsubscribe from
> the former, and UNLISTEN SIMILAR TO for the latter.  As you said in the
> original email:
>
> > Allow *UNLISTEN SIMILAR TO 'xxx'* which would unregister matching
> listeners. To me this feels confusing therefore it is not in the patch.
>
> I agree, starting to match the patterns themselves would be confusing.  So
> I think we should use that syntax for unsubscribing from patterns.  But
> others should feel free to express their opinions on this.
>
> Secondly -- and this is a kind of continuation to my previous point of
> conflating patterns and non-patterns -- I don't think you can get away with
> not changing the interface for pg_listening_channels().  Not knowing which
> ones are compared byte-for-byte and which ones are patterns just seems
> weird.
>
>
> As for the patch itself, I have a couple of comments.  I'm writing this
> based on the latest commit in your git branch, commit
> fded070f2a56024f931b9a0f174320eebc362458.
>
> In queue_listen(), the new variables would be better declared at the
> innermost scope possible.  The datum is only used if isSimilarToPattern is
> true, errormsg only if compile_regex didn't return REG_OKAY, etc..
>
> I found this comment confusing at first: "If compiled RE was not applied
> as a listener then it is freed at transaction commit."  The past tense
> makes it seem like something that has already happened when that code runs,
> when in reality it happens later in the transaction.
>
> I'm not a fan of the dance you're doing with pcompreg.  I think it would
> be better to 

Re: [HACKERS] [PATCH] Pattern based listeners for asynchronous messaging (LISTEN/NOTIFY)

[Marko Tiikkaja]

Hi Markus,

On Sun, Aug 20, 2017 at 9:56 PM, Markus Sintonen 
wrote:

> I also encountered this when I built it with different configuration. I
> attached updated patch with the correct number of arguments to
> 'similar_escape'. I also added preliminary documentation to the patch.
> (Unfortunately unable to currently compile the documentation for testing
> purpose on Windows probably because of commit https://github.com/post
> gres/postgres/commit/510074f9f0131a04322d6a3d2a51c87e6db243f9. I followed
> https://www.postgresql.org/docs/devel/static/install-windows
> -full.html#idm45412738673840.)
>
> What do you think about the syntax? There was a suggestion to specify type
> of the pattern (eg ltree extension) but to me this feels like a overkill.
> One option here would be eg:
> LISTEN PATTERN 'foo%' TYPE 'similar'
> LISTEN PATTERN 'foo.*' TYPE 'ltree'
>

Not that my opinion matters, but I think we should pick one pattern style
and stick to it.  SIMILAR TO doesn't seem like the worst choice.  ltree
seems useless.

As for the rest of the interface..

First, I think mixing patterns and non-patterns is weird.  This is apparent
in at least two cases:

marko=# listen "foo%";
LISTEN
marko=# listen similar to 'foo%';
LISTEN
marko=# select * from pg_listening_channels();
 pg_listening_channels
---
 foo%
(1 row)

-- Not actually listening on the pattern.  Confusion.

The second case being the way UNLISTEN can be used to unlisten patterns,
too.  It kind of makes sense given that you can't really end up with both a
channel name and a pattern with the same source string, but it's still
weird.  I think it would be much better to keep these completely separate
so that you could be listening on both the channel "foo%" and the pattern
'foo%' at the same time, and you'd use a bare UNLISTEN to unsubscribe from
the former, and UNLISTEN SIMILAR TO for the latter.  As you said in the
original email:

> Allow *UNLISTEN SIMILAR TO 'xxx'* which would unregister matching
listeners. To me this feels confusing therefore it is not in the patch.

I agree, starting to match the patterns themselves would be confusing.  So
I think we should use that syntax for unsubscribing from patterns.  But
others should feel free to express their opinions on this.

Secondly -- and this is a kind of continuation to my previous point of
conflating patterns and non-patterns -- I don't think you can get away with
not changing the interface for pg_listening_channels().  Not knowing which
ones are compared byte-for-byte and which ones are patterns just seems
weird.


As for the patch itself, I have a couple of comments.  I'm writing this
based on the latest commit in your git branch, commit
fded070f2a56024f931b9a0f174320eebc362458.

In queue_listen(), the new variables would be better declared at the
innermost scope possible.  The datum is only used if isSimilarToPattern is
true, errormsg only if compile_regex didn't return REG_OKAY, etc..

I found this comment confusing at first: "If compiled RE was not applied as
a listener then it is freed at transaction commit."  The past tense makes
it seem like something that has already happened when that code runs, when
in reality it happens later in the transaction.

I'm not a fan of the dance you're doing with pcompreg.  I think it would be
better to optimistically allocate the ListenAction struct and compile
directly into actrec->compiledRegex.

The changed DEBUG1 line in Async_Listen should include whether it's a
pattern or not.

I don't understand why the return value of Exec_UnlistenAllCommit() was
changed at all.  Why do we need to do something different based on whether
listenChannels was empty or not?  The same goes for Exec_UnlistenCommit.

This looks wrong in isolationtester.c:

@@ -487,7 +488,7 @@ run_permutation(TestSpec *testspec, int nsteps, Step
**steps)
res = PQexec(conns[0], testspec->setupsqls[i]);
if (PQresultStatus(res) == PGRES_TUPLES_OK)
{
-   printResultSet(res);
+   printResultSet(res, conns[i + 1]);

(conns[0] vs. conns[i + 1]).

Moving to Waiting on Author.


.m

Fwd: [HACKERS] [PATCH] Pattern based listeners for asynchronous messaging (LISTEN/NOTIFY)

[Markus Sintonen]

I also encountered this when I built it with different configuration. I
attached updated patch with the correct number of arguments to
'similar_escape'. I also added preliminary documentation to the patch.
(Unfortunately unable to currently compile the documentation for testing
purpose on Windows probably because of commit https://github.com/
postgres/postgres/commit/510074f9f0131a04322d6a3d2a51c87e6db243f9. I
followed https://www.postgresql.org/docs/devel/static/install-
windows-full.html#idm45412738673840.)

What do you think about the syntax? There was a suggestion to specify type
of the pattern (eg ltree extension) but to me this feels like a overkill.
One option here would be eg:
LISTEN PATTERN 'foo%' TYPE 'similar'
LISTEN PATTERN 'foo.*' TYPE 'ltree'
... and so on

BR
-Markus

2017-08-19 2:36 GMT+03:00 Thomas Munro :

> On Tue, Aug 1, 2017 at 8:13 AM, Markus Sintonen
>  wrote:
> > This patch adds an ability to use patterns in LISTEN commands. Patch uses
> > 'SIMILAR TO' patterns for matching NOTIFY channel names
> > (https://www.postgresql.org/docs/9.0/static/functions-matchi
> ng.html#FUNCTIONS-SIMILARTO-REGEXP).
> >
> > This patch is related to old discussion in
> > https://www.postgresql.org/message-id/52693fc5.7070...@gmail.com. This
> > discussion contains the reasoning behind the pattern based matching of
> the
> > channel names.
>
> Nice idea.
>
> The "async" regression test consistently crashes on my FreeBSD box
> when built with -O2.  It doesn't crash on another system I tried, and
> I think that's just luck, because this:
>
> +   /* convert to regex pattern */
> +   datum = DirectFunctionCall1(similar_escape,
> CStringGetTextDatum(pattern));
>
> ... is calling a function that takes two arguments, but passing only
> one.  The second argument is random junk, so similar_escape bombs when
> it does this:
>
> esc_text = PG_GETARG_TEXT_PP(1);
>
> --
> Thomas Munro
> http://www.enterprisedb.com
>


listen-pattern.patch
Description: Binary data

-- 
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] Pattern based listeners for asynchronous messaging (LISTEN/NOTIFY)

[Thomas Munro]

On Tue, Aug 1, 2017 at 8:13 AM, Markus Sintonen
 wrote:
> This patch adds an ability to use patterns in LISTEN commands. Patch uses
> 'SIMILAR TO' patterns for matching NOTIFY channel names
> (https://www.postgresql.org/docs/9.0/static/functions-matching.html#FUNCTIONS-SIMILARTO-REGEXP).
>
> This patch is related to old discussion in
> https://www.postgresql.org/message-id/52693fc5.7070...@gmail.com. This
> discussion contains the reasoning behind the pattern based matching of the
> channel names.

Nice idea.

The "async" regression test consistently crashes on my FreeBSD box
when built with -O2.  It doesn't crash on another system I tried, and
I think that's just luck, because this:

+   /* convert to regex pattern */
+   datum = DirectFunctionCall1(similar_escape,
CStringGetTextDatum(pattern));

... is calling a function that takes two arguments, but passing only
one.  The second argument is random junk, so similar_escape bombs when
it does this:

esc_text = PG_GETARG_TEXT_PP(1);

(lldb) bt
* thread #1, name = 'postgres', stop reason = signal SIGSEGV
  * frame #0: postgres`pg_detoast_datum_packed(datum=0x)
at fmgr.c:1865
frame #1: postgres`similar_escape(fcinfo=0x7fffd080) at regexp.c:686
frame #2: postgres`DirectFunctionCall1Coll(func=(postgres`similar_escape
at regexp.c:659), collation=, arg1=) at
fmgr.c:717
frame #3: postgres`queue_listen(action=LISTEN_LISTEN,
pattern="notify_%", isSimilarToPattern=) at async.c:671
frame #4: postgres`standard_ProcessUtility(pstmt=0x000801824df0,
queryString="LISTEN SIMILAR TO 'notify_%';",
context=PROCESS_UTILITY_TOPLEVEL, params=0x,
queryEnv=0x, dest=0x000801824ee8,
completionTag=) at utility.c:633
frame #5: postgres`PortalRunUtility(portal=0x000801960040,
pstmt=0x000801824df0, isTopLevel='\x01',
setHoldSnapshot=, dest=, completionTag="")
at pquery.c:1178
frame #6: postgres`PortalRunMulti(portal=0x000801960040,
isTopLevel='\x01', setHoldSnapshot='\0', dest=0x000801824ee8,
altdest=0x000801824ee8, completionTag="") at pquery.c:0
frame #7: postgres`PortalRun(portal=0x000801960040,
count=, isTopLevel='\x01', run_once=,
dest=, altdest=,
completionTag=) at pquery.c:799
frame #8: postgres`exec_simple_query(query_string="LISTEN SIMILAR
TO 'notify_%';") at postgres.c:1099
frame #9: postgres`PostgresMain(argc=,
argv=, dbname=, username=) at
postgres.c:0
frame #10: postgres`PostmasterMain [inlined] BackendRun at postmaster.c:4357
frame #11: postgres`PostmasterMain [inlined] BackendStartup at
postmaster.c:4029
frame #12: postgres`PostmasterMain at postmaster.c:1753
frame #13: postgres`PostmasterMain(argc=,
argv=0x0008018d11c0) at postmaster.c:1361
frame #14: postgres`main(argc=, argv=)
at main.c:228
frame #15: 0x004823af postgres`_start + 383

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
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] Pattern based listeners for asynchronous messaging (LISTEN/NOTIFY)

[Markus Sintonen]

The following does not work:
LISTEN 'foo%'
Neither this:
LISTEN SIMILAR TO "foo%"
This works:
LISTEN "foo%"
But it does not act as a pattern.

We could change the SIMILAR TO something like following (accepting also
type of the pattern), for example:
LISTEN PATTERN 'foo%' TYPE 'similar'
LISTEN PATTERN 'foo*' TYPE 'ltree'
Or
LISTEN PATTERN 'foo%' USING 'similar'
Or
LISTEN MATCH 'foo%' USING 'similar'
Or
LISTEN MATCH 'foo%' TYPE 'similar'

Documentation is coming up.

2017-07-31 23:30 GMT+03:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> On 7/31/17 16:13, Markus Sintonen wrote:
> > This patch has no know backward compatibility issues with the existing
> > /LISTEN///UNLISTEN/ features. This is because patch extends the existing
> > syntax by accepting quoted strings which define the patterns as opposed
> > to the existing SQL literals.
>
> I don't see that in the patch.  Your patch changes the syntax LISTEN
> ColId to mean a regular expression.
>
> Even then, having LISTEN "foo" and LISTEN 'foo' mean different things
> would probably be confusing.
>
> I would think about specifying an operator somewhere in the syntax, like
> you have with LISTEN SIMILAR TO.  It would even be nice if a
> non-built-in operator could be used for matching names.
>
> Documentation is missing in the patch.
>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] [PATCH] Pattern based listeners for asynchronous messaging (LISTEN/NOTIFY)

[Peter Eisentraut]

On 7/31/17 16:13, Markus Sintonen wrote:
> This patch has no know backward compatibility issues with the existing
> /LISTEN///UNLISTEN/ features. This is because patch extends the existing
> syntax by accepting quoted strings which define the patterns as opposed
> to the existing SQL literals.

I don't see that in the patch.  Your patch changes the syntax LISTEN
ColId to mean a regular expression.

Even then, having LISTEN "foo" and LISTEN 'foo' mean different things
would probably be confusing.

I would think about specifying an operator somewhere in the syntax, like
you have with LISTEN SIMILAR TO.  It would even be nice if a
non-built-in operator could be used for matching names.

Documentation is missing in the patch.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] [PATCH] Pattern based listeners for asynchronous messaging (LISTEN/NOTIFY)

[Markus Sintonen]

Hi

This patch adds an ability to use patterns in *LISTEN* commands. Patch uses
'*SIMILAR TO*' patterns for matching *NOTIFY* channel names (
https://www.postgresql.org/docs/9.0/static/functions-matching.html#FUNCTIONS-SIMILARTO-REGEXP
).

This patch is related to old discussion in
https://www.postgresql.org/message-id/52693fc5.7070...@gmail.com. This
discussion contains the reasoning behind the pattern based matching of the
channel names.

Patch allows the following.
The listener is registered with following command, for example:
*LISTEN SIMILAR TO 'test%';*
which would match and receive a message from this example notfication:
*NOTIFY test_2;*
Unlistening the above registered pattern:
*UNLISTEN 'test%';*
More examples can be seen from the added regress and isolation tests.
Note that *UNLISTEN* does not allow pattern based unlistening of the
registered listeners. It merely matches the registered pattern by simple
string comparison.

This patch has no know backward compatibility issues with the existing
*LISTEN*/*UNLISTEN* features. This is because patch extends the existing
syntax by accepting quoted strings which define the patterns as opposed to
the existing SQL literals.

The patch also extends the isolationtester by adding an ability to monitor
registered notify messages. This is used to test the existing features as
well as the new pattern based feature. (Does not affect other existing
tests)

Further considerations for this patch:
- Change pattern type, alternatives would be e.g. *LIKE* or POSIX patterns
- Remove '*SIMILAR TO*' from the command (just use quoted string in
form of *LISTEN
'test_%'*)
- Allow *UNLISTEN SIMILAR TO 'xxx'* which would unregister matching
listeners. To me this feels confusing therefore it is not in the patch.

Some notes about patch:
- Most of the changes in async.c
- Regular expression is compiled and finally stored in top memory context
- RE compilation (+error check) happens before transaction commit
- RE is compiled in current transaction memory context from where it is
copied to top memory context on transaction commit (when everything first
succeeds)
- Compiled REs in current transaction are freed on transaction abort
- Compiled REs that were not applied as listeners (duplicates) are freed on
transaction commit
- REs freed when unlistened
- No obvious performance impacts (e.g. normal channel name listening uses
just strcmp)

Thoughts?

Best regards
Markus


listen-pattern.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers