Thread: [PATCH] Allow UNLISTEN during recovery
Hi all,
Here is a tiny patch removing PreventCommandDuringRecovery() for UNLISTEN. See previous discussion in https://www.postgresql.org/message-id/CADT4RqBweu7QKRYAYzeRW77b%2BMhJdUikNe45m%2BfL4GJSq_u2Fg%40mail.gmail.com.
In a nutshell, this prevents an error being raised when UNLISTEN is issued during recovery. The operation is a no-op (since LISTEN is still disallowed). This logic here is that some clients (namely Npgsql) issue UNLISTEN * to clear connection state (in the connection pool), but this needlessly breaks when the backend is in recovery.
On a related note, there currently doesn't seem to be a good way for clients to know whether the backend is in recovery. As a backend can come out of recovery at any point, perhaps an asynchronous ParameterStatus announcing this state change could be useful.
Hopefully this also qualifies for backporting to earlier version branches.
Shay
Attachment
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: not tested Spec compliant: not tested Documentation: tested, failed Hi! I read through the discussion. I agree with the idea here. I think if DISCARD ALL is allowed during hot standby mode, andit includes UNLISTEN *, only UNLISTEN * should also be allowed. It seems this patch fixes this, but I could not test it(I do not know how to force this state). I would go further though, and I would also allow UNLISTEN a. Because also DISCARDallows discarding only part of resources. So patch looks good to me, but documentation changes are missing from it. UNLISTEN should be removed from the list of commandsnot allowed and moved to the list of those which are allowed [1]. Moreover, src/test/regress/sql/hs_standby_allowed.sqland src/test/regress/sql/hs_standby_disallowed.sql tests should be updated basedon these changes in the patch. What is surprising to me is that make check-world does not fail with this change, butthere is an explicit check for UNLISTEN *. So does this mean those tests are not run or does it mean that this patch doesnot fix the problem? [1] https://www.postgresql.org/docs/current/hot-standby.html#HOT-STANDBY-USERS Mitar The new status of this patch is: Waiting on Author
Mitar, thanks for giving this your attention!
So patch looks good to me, but documentation changes are missing from it. UNLISTEN should be removed from the list of commands not allowed and moved to the list of those which are allowed [1]. Moreover, src/test/regress/sql/hs_standby_allowed.sql and src/test/regress/sql/hs_standby_disallowed.sql tests should be updated based on these changes in the patch. What is surprising to me is that make check-world does not fail with this change, but there is an explicit check for UNLISTEN *. So does this mean those tests are not run or does it mean that this patch does not fix the problem?
I've made the requested changes to the docs and to the regression tests.
I think that specifically the standby regression tests do not get executed by check-world - see section https://www.postgresql.org/docs/current/regress-run.html#id-1.6.20.5.8. I'm guessing this should be executed as part of the build verification pipeline for patches, but I don't know anything about the PostgreSQL build infrastructure.
Unfortunately I'm extremely tight for time at the moment and don't have time to do the appropriate hot standby setup to test this... As the patch is pretty straightforward, and since I'm hoping you guys execute the tests on your end, I hope that's acceptable. If it's absolutely necessary for me to test the patch locally, let me know and I'll do so.
Attachment
Hi! On Tue, Jan 15, 2019 at 10:17 AM Shay Rojansky <roji@roji.org> wrote: > Unfortunately I'm extremely tight for time at the moment and don't have time to do the appropriate hot standby setup totest this... As the patch is pretty straightforward, and since I'm hoping you guys execute the tests on your end, I hopethat's acceptable. If it's absolutely necessary for me to test the patch locally, let me know and I'll do so. I would definitely prefer if you could run the tests. You might discover that your patch does not sufficiently address tests. Please do so and confirm here that it works for you and then I can do another review. Mitar -- http://mitar.tnode.com/ https://twitter.com/mitar_m
On Tue, Jan 15, 2019 at 10:17 AM Shay Rojansky <roji@roji.org> wrote:
> Unfortunately I'm extremely tight for time at the moment and don't have time to do the appropriate hot standby setup to test this... As the patch is pretty straightforward, and since I'm hoping you guys execute the tests on your end, I hope that's acceptable. If it's absolutely necessary for me to test the patch locally, let me know and I'll do so.
I would definitely prefer if you could run the tests. You might
discover that your patch does not sufficiently address tests. Please
do so and confirm here that it works for you and then I can do another
review.
Attachment
Shay Rojansky <roji@roji.org> writes:
> Thanks for insisting - I ended up setting up the environment and running
> the tests, and discovering that some test-related changes were missing.
> Here's a 3rd version of the patch. Hope this is now in good shape, let me
> know if you think anything else needs to be done.
Lotta work for a one-line code change, eh? Pushed now.
regards, tom lane
> Thanks for insisting - I ended up setting up the environment and running
> the tests, and discovering that some test-related changes were missing.
> Here's a 3rd version of the patch. Hope this is now in good shape, let me
> know if you think anything else needs to be done.
Lotta work for a one-line code change, eh? Pushed now.
Yes, especially when you're new to building and testing PostgreSQL :) Thanks for accepting!
On Sat, 26 Jan 2019 at 02:17, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Shay Rojansky <roji@roji.org> writes:
> Thanks for insisting - I ended up setting up the environment and running
> the tests, and discovering that some test-related changes were missing.
> Here's a 3rd version of the patch. Hope this is now in good shape, let me
> know if you think anything else needs to be done.
Lotta work for a one-line code change, eh? Pushed now.
Good decision, thanks.
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services