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

Hi Markus,

On Sun, Aug 20, 2017 at 9:56 PM, Markus Sintonen <markus.sintonen@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/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'

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