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

Hi Markus,

On Sun, Aug 20, 2017 at 9:56 PM, Markus Sintonen <markus(dot)sintonen(at)gmail(dot)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 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