Re: [PATCH] TODO “Allow LISTEN on patterns”
| От | Emanuel Calvo |
|---|---|
| Тема | Re: [PATCH] TODO “Allow LISTEN on patterns” |
| Дата | |
| Msg-id | CAJeAsn_6u+Ti+tkhOuJ4wr+c5TF6AjSVxaQW=cNHf=TXe9gVBw@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: [PATCH] TODO “Allow LISTEN on patterns” (Alexander Cheshev <alex.cheshev@gmail.com>) |
| Список | pgsql-hackers |
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.
# 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
In the case of the inner levels creating a channel name too long, it may probably sane to just
Another observation, probably not strictly related to this patch itself but the async-notify tests, is that there is no test for
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
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
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,
Regards,
El lun, 15 jul 2024 a las 12:59, Alexander Cheshev (<alex.cheshev@gmail.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 CheshevOn Sat, 13 Jul 2024 at 13:26, Alexander Cheshev <alex.cheshev@gmail.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.
Regards,
Alexander CheshevRegards,Alexander CheshevOn Tue, 9 Jul 2024 at 11:01, Emanuel Calvo <3manuek@gmail.com> wrote:Hello there,El vie, 15 mar 2024 a las 9:01, Alexander Cheshev (<alex.cheshev@gmail.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'tseem 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 havethe 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,--
В списке pgsql-hackers по дате отправления: