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

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 <marko(at)joh(dot)to>:

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