Re: [PATCH] TODO “Allow LISTEN on patterns”

Hi Alexander,

I did a review on the new patch version and I observed that the identifier
passed to the LISTEN command is handled differently between outer and inner
levels.

When the outer level exceeds the 64 characters limitation, the outer level
of the
channel name is truncated, but leaves the inner levels in the channel name
due
that isn't parsed in the same way.

Also, even if the outer level isn't truncated, it is allowed to add
channels names
that exceeds the allowed identifier size.

It can be reproduced just by:

# LISTEN a.a.a.a.a.lot.of.levels..; -- this doesn't fail at LISTEN,
but fails in NOTIFY due to channel name too long

In the following, the outer level is truncated, but it doesn't cut out the
inner levels. This leaves
listening channels that cannot receive any notifications in the queue:

# LISTEN
notify_async_channel_name_too_long____________________________________.a.a.
...
NOTICE: identifier .... will be truncated

# select substring(c.channel,0,66), length(c.channel) from
pg_listening_channels() c(channel) where length(c.channel) > 64;
substring |
notify_async_channel_name_too_long_____________________________.a
length | 1393

I guess that the expected behavior would be that if the outer level is
truncated, the rest of the
channel name should be ignored, as there won't be possible to notify it
anyway.

In the case of the inner levels creating a channel name too long, it may
probably sane to just
check the length of the entire identifier, and truncate -- ensuring that
channel name doesn't
end with the level separator.

Another observation, probably not strictly related to this patch itself but
the async-notify tests, is that there is no test for
"payload too long". Probably there is a reason on why isn't in the specs?

Regards,

El lun, 15 jul 2024 a las 12:59, Alexander Cheshev (<alex(dot)cheshev(at)gmail(dot)com>)
escribió:

> Hi Emanuel,
>
> Changed implementation of the function Exec_UnlistenCommit . v2 of the
> path contained a bug in the function Exec_UnlistenCommit (added a test case
> for that) and also it was not implemented in natural to C form using
> pointers. Now it looks fine and works as expected.
>
> In the previous email I forgot to mention that the new implementation of
> the function Exec_UnlistenCommit has the same space and time complexities
> as the original implementation (which doesn't support wildcards).
>
> Regards,
> Alexander Cheshev
>
>
> On Sat, 13 Jul 2024 at 13:26, Alexander Cheshev <alex(dot)cheshev(at)gmail(dot)com>
> wrote:
>
>> Hi Emanuel,
>>
>> I did a test over the "UNLISTEN >" behavior, and I'm not sure if this is
>>> expected.
>>> This command I assume should free all the listening channels, however,
>>> it doesn't
>>> seem to do so:
>>
>>
>> TODO “Allow LISTEN on patterns” [1] is a bit vague about that feature. So
>> I didn't implement it in the first version of the patch. Also I see that I
>> made a mistake in the documentation and mentioned that it is actually
>> supported. Sorry for the confusion.
>>
>> Besides obvious reasons I think that your finding is especially
>> attractive for the following reason. We have an UNLISTEN * command. If we
>> replace > with * in the patch (which I actually did in the new version of
>> the patch) then we have a generalisation of the above command. For example,
>> UNLISTEN a* cancels registration on all channels which start with a.
>>
>> I attached to the email the new version of the patch which supports the
>> requested feature. Instead of > I use * for the reason which I mentioned
>> above. Also I added test cases, changed documentation, etc.
>>
>> I appreciate your work, Emanuel! If you have any further findings I will
>> be glad to adjust the patch accordingly.
>>
>> [1]
>> https://www.postgresql.org/message-id/flat/52693FC5.7070507%40gmail.com
>>
>> Regards,
>> Alexander Cheshev
>>
>> Regards,
>> Alexander Cheshev
>>
>>
>> On Tue, 9 Jul 2024 at 11:01, Emanuel Calvo <3manuek(at)gmail(dot)com> wrote:
>>
>>>
>>> Hello there,
>>>
>>>
>>> El vie, 15 mar 2024 a las 9:01, Alexander Cheshev (<
>>> alex(dot)cheshev(at)gmail(dot)com>) escribió:
>>>
>>>> Hello Hackers,
>>>>
>>>> I have implemented TODO “Allow LISTEN on patterns” [1] and attached
>>>> the patch to the email. The patch basically consists of the following
>>>> two parts.
>>>>
>>>> 1. Support wildcards in LISTEN command
>>>>
>>>> Notification channels can be composed of multiple levels in the form
>>>> ‘a.b.c’ where ‘a’, ‘b’ and ‘c’ are identifiers.
>>>>
>>>> Listen channels can be composed of multiple levels in the form ‘a.b.c’
>>>> where ‘a’, ‘b’ and ‘c’ are identifiers which can contain the following
>>>> wildcards:
>>>> * ‘%’ matches everything until the end of a level. Can only appear
>>>> at the end of a level. For example, the notification channels ‘a.b.c’,
>>>> ‘a.bc.c’ match against the listen channel ‘a.b%.c’.
>>>> * ‘>’ matches everything to the right. Can only appear at the end of
>>>> the last level. For example, the notification channels ‘a.b’, ‘a.bc.d’
>>>> match against the listen channel ‘a.b>’.
>>>>
>>>>
>>> I did a test over the "UNLISTEN >" behavior, and I'm not sure if this is
>>> expected.
>>> This command I assume should free all the listening channels, however,
>>> it doesn't
>>> seem to do so:
>>>
>>> postgres=# LISTEN device1.alerts.%;
>>> LISTEN
>>> postgres=# ;
>>> Asynchronous notification "device1.alerts.temp" with payload "80"
>>> received from server process with PID 237.
>>> postgres=# UNLISTEN >;
>>> UNLISTEN
>>> postgres=# ; -- Here I send a notification over the same channel
>>> Asynchronous notification "device1.alerts.temp" with payload "80"
>>> received from server process with PID 237.
>>>
>>> The same happens with "UNLISTEN %;", although I'm not sure if this
>>> should have
>>> the same behavior.
>>>
>>> It stops listening correctly if I do explicit UNLISTEN (exact channel
>>> matching).
>>>
>>> I'll be glad to conduct more tests or checks on this.
>>>
>>> Cheers,
>>>
>>>
>>> --
>>> --
>>> Emanuel Calvo
>>> Database Engineering
>>> https://tr3s.ma/aobut
>>>
>>>

--
--
Emanuel Calvo
https://tr3s.ma/ <https://tr3s.ma/about>