[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

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, and it 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 DISCARD allows 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 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?

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

Hi!

On Tue, Jan 15, 2019 at 10:17 AM Shay Rojansky <roji(at)roji(dot)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.

Mitar

--
http://mitar.tnode.com/
https://twitter.com/mitar_m

>
> On Tue, Jan 15, 2019 at 10:17 AM Shay Rojansky <roji(at)roji(dot)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.
>

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.

Shay Rojansky <roji(at)roji(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Shay Rojansky <roji(at)roji(dot)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/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services