[PATCH] Allow UNLISTEN during recovery

Started by Shay Rojanskyabout 7 years ago8 messages

roji@roji.org

about 7 years ago

1 attachment(s)

Hi all,

Here is a tiny patch removing PreventCommandDuringRecovery() for UNLISTEN.
See previous discussion in
/messages/by-id/CADT4RqBweu7QKRYAYzeRW77b+MhJdUikNe45m+fL4GJSq_u2Fg@mail.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

Attachments:

allow-UNLISTEN-during-recovery.patchtext/x-patch; charset=US-ASCII; name=allow-UNLISTEN-during-recovery.patchDownload

diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 970c94ee80..3efd262cb8 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -629,7 +629,7 @@ standard_ProcessUtility(PlannedStmt *pstmt,
 			{
 				UnlistenStmt *stmt = (UnlistenStmt *) parsetree;
 
-				PreventCommandDuringRecovery("UNLISTEN");
+				/* allow UNLISTEN during recovery, which is a noop */
 				CheckRestrictedOperation("UNLISTEN");
 				if (stmt->conditionname)
 					Async_Unlisten(stmt->conditionname);

Mi Tar

mmitar@gmail.com

about 7 years ago

In reply to: Shay Rojansky (#1)

Re: [PATCH] Allow UNLISTEN during recovery

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]https://www.postgresql.org/docs/current/hot-standby.html#HOT-STANDBY-USERS. 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

Shay Rojansky

roji@roji.org

almost 7 years ago

In reply to: Mi Tar (#2)

1 attachment(s)

Re: [PATCH] Allow UNLISTEN during recovery

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.

Attachments:

0002-allow-UNLISTEN-during-recovery.patchtext/x-patch; charset=US-ASCII; name=0002-allow-UNLISTEN-during-recovery.patchDownload

From 8c816354e820bf3d0be69d55dbf0052b1d27feeb Mon Sep 17 00:00:00 2001
From: Shay Rojansky <roji@roji.org>
Date: Tue, 15 Jan 2019 18:49:40 +0100
Subject: [PATCH] Allow unlisten when in hot standby:wq

---
 doc/src/sgml/high-availability.sgml            | 16 ++++++++++------
 src/backend/tcop/utility.c                     |  2 +-
 src/test/regress/sql/hs_standby_allowed.sql    |  4 ++++
 src/test/regress/sql/hs_standby_disallowed.sql |  2 --
 4 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index 4882b20828..bb1c86f73e 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -1767,6 +1767,11 @@ if (!triggered)
        Plugins and extensions - <command>LOAD</command>
       </para>
      </listitem>
+     <listitem>
+      <para>
+       UNLISTEN
+      </para>
+     </listitem>
     </itemizedlist>
    </para>
 
@@ -1856,18 +1861,17 @@ if (!triggered)
      </listitem>
      <listitem>
       <para>
-       <command>LISTEN</command>, <command>UNLISTEN</command>, <command>NOTIFY</command>
+       <command>LISTEN</command>, <command>NOTIFY</command>
       </para>
      </listitem>
     </itemizedlist>
    </para>
 
    <para>
-    In normal operation, <quote>read-only</quote> transactions are allowed to
-    use <command>LISTEN</command>, <command>UNLISTEN</command>, and
-    <command>NOTIFY</command>, so Hot Standby sessions operate under slightly tighter
-    restrictions than ordinary read-only sessions.  It is possible that some
-    of these restrictions might be loosened in a future release.
+    In normal operation, <quote>read-only</quote> transactions are allowed to use
+    <command>LISTEN</command>, and <command>NOTIFY</command>, so Hot Standby sessions
+    operate under slightly tighter restrictions than ordinary read-only sessions.
+    It is possible that some of these restrictions might be loosened in a future release.
    </para>
 
    <para>
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 27ae6be751..44136060d6 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -629,7 +629,7 @@ standard_ProcessUtility(PlannedStmt *pstmt,
 			{
 				UnlistenStmt *stmt = (UnlistenStmt *) parsetree;
 
-				PreventCommandDuringRecovery("UNLISTEN");
+				/* allow UNLISTEN during recovery, which is a noop */
 				CheckRestrictedOperation("UNLISTEN");
 				if (stmt->conditionname)
 					Async_Unlisten(stmt->conditionname);
diff --git a/src/test/regress/sql/hs_standby_allowed.sql b/src/test/regress/sql/hs_standby_allowed.sql
index a33199dbbd..1f1cdf5a00 100644
--- a/src/test/regress/sql/hs_standby_allowed.sql
+++ b/src/test/regress/sql/hs_standby_allowed.sql
@@ -110,6 +110,10 @@ LOCK hs1 IN ROW SHARE MODE;
 LOCK hs1 IN ROW EXCLUSIVE MODE;
 COMMIT;
 
+-- UNLISTEN
+unlisten a;
+unlisten *;
+
 -- LOAD
 -- should work, easier if there is no test for that...
 
diff --git a/src/test/regress/sql/hs_standby_disallowed.sql b/src/test/regress/sql/hs_standby_disallowed.sql
index 21bbf526b7..a470600eec 100644
--- a/src/test/regress/sql/hs_standby_disallowed.sql
+++ b/src/test/regress/sql/hs_standby_disallowed.sql
@@ -88,8 +88,6 @@ COMMIT;
 -- Listen
 listen a;
 notify a;
-unlisten a;
-unlisten *;
 
 -- disallowed commands
 
-- 
2.19.1

Mitar

mmitar@gmail.com

almost 7 years ago

In reply to: Shay Rojansky (#3)

Re: [PATCH] Allow UNLISTEN during recovery

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

Shay Rojansky

roji@roji.org

almost 7 years ago

In reply to: Mitar (#4)

1 attachment(s)

Re: [PATCH] Allow UNLISTEN during recovery

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.

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.

Attachments:

0003-Allow-UNLISTEN-during-recovery.patchtext/x-patch; charset=US-ASCII; name=0003-Allow-UNLISTEN-during-recovery.patchDownload

From c5497cc4f7fbad4eafecfa72bc99dfebacd0f9bd Mon Sep 17 00:00:00 2001
From: Shay Rojansky <roji@roji.org>
Date: Tue, 15 Jan 2019 18:49:40 +0100
Subject: [PATCH] Allow UNLISTEN during recovery

---
 doc/src/sgml/high-availability.sgml              | 16 ++++++++++------
 src/backend/tcop/utility.c                       |  2 +-
 src/test/regress/expected/hs_standby_allowed.out |  3 +++
 .../regress/expected/hs_standby_disallowed.out   |  4 ----
 src/test/regress/sql/hs_standby_allowed.sql      |  4 ++++
 src/test/regress/sql/hs_standby_disallowed.sql   |  2 --
 6 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index 4882b20828..bb1c86f73e 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -1767,6 +1767,11 @@ if (!triggered)
        Plugins and extensions - <command>LOAD</command>
       </para>
      </listitem>
+     <listitem>
+      <para>
+       UNLISTEN
+      </para>
+     </listitem>
     </itemizedlist>
    </para>
 
@@ -1856,18 +1861,17 @@ if (!triggered)
      </listitem>
      <listitem>
       <para>
-       <command>LISTEN</command>, <command>UNLISTEN</command>, <command>NOTIFY</command>
+       <command>LISTEN</command>, <command>NOTIFY</command>
       </para>
      </listitem>
     </itemizedlist>
    </para>
 
    <para>
-    In normal operation, <quote>read-only</quote> transactions are allowed to
-    use <command>LISTEN</command>, <command>UNLISTEN</command>, and
-    <command>NOTIFY</command>, so Hot Standby sessions operate under slightly tighter
-    restrictions than ordinary read-only sessions.  It is possible that some
-    of these restrictions might be loosened in a future release.
+    In normal operation, <quote>read-only</quote> transactions are allowed to use
+    <command>LISTEN</command>, and <command>NOTIFY</command>, so Hot Standby sessions
+    operate under slightly tighter restrictions than ordinary read-only sessions.
+    It is possible that some of these restrictions might be loosened in a future release.
    </para>
 
    <para>
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 27ae6be751..44136060d6 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -629,7 +629,7 @@ standard_ProcessUtility(PlannedStmt *pstmt,
 			{
 				UnlistenStmt *stmt = (UnlistenStmt *) parsetree;
 
-				PreventCommandDuringRecovery("UNLISTEN");
+				/* allow UNLISTEN during recovery, which is a noop */
 				CheckRestrictedOperation("UNLISTEN");
 				if (stmt->conditionname)
 					Async_Unlisten(stmt->conditionname);
diff --git a/src/test/regress/expected/hs_standby_allowed.out b/src/test/regress/expected/hs_standby_allowed.out
index 526f88f2be..00b8faf9eb 100644
--- a/src/test/regress/expected/hs_standby_allowed.out
+++ b/src/test/regress/expected/hs_standby_allowed.out
@@ -208,6 +208,9 @@ LOCK hs1 IN ACCESS SHARE MODE;
 LOCK hs1 IN ROW SHARE MODE;
 LOCK hs1 IN ROW EXCLUSIVE MODE;
 COMMIT;
+-- UNLISTEN
+UNLISTEN a;
+UNLISTEN *;
 -- LOAD
 -- should work, easier if there is no test for that...
 -- ALLOWED COMMANDS
diff --git a/src/test/regress/expected/hs_standby_disallowed.out b/src/test/regress/expected/hs_standby_disallowed.out
index bc117413ff..dff0953e9a 100644
--- a/src/test/regress/expected/hs_standby_disallowed.out
+++ b/src/test/regress/expected/hs_standby_disallowed.out
@@ -118,10 +118,6 @@ listen a;
 ERROR:  cannot execute LISTEN during recovery
 notify a;
 ERROR:  cannot execute NOTIFY during recovery
-unlisten a;
-ERROR:  cannot execute UNLISTEN during recovery
-unlisten *;
-ERROR:  cannot execute UNLISTEN during recovery
 -- disallowed commands
 ANALYZE hs1;
 ERROR:  cannot execute ANALYZE during recovery
diff --git a/src/test/regress/sql/hs_standby_allowed.sql b/src/test/regress/sql/hs_standby_allowed.sql
index a33199dbbd..6debddc5e9 100644
--- a/src/test/regress/sql/hs_standby_allowed.sql
+++ b/src/test/regress/sql/hs_standby_allowed.sql
@@ -110,6 +110,10 @@ LOCK hs1 IN ROW SHARE MODE;
 LOCK hs1 IN ROW EXCLUSIVE MODE;
 COMMIT;
 
+-- UNLISTEN
+UNLISTEN a;
+UNLISTEN *;
+
 -- LOAD
 -- should work, easier if there is no test for that...
 
diff --git a/src/test/regress/sql/hs_standby_disallowed.sql b/src/test/regress/sql/hs_standby_disallowed.sql
index 21bbf526b7..a470600eec 100644
--- a/src/test/regress/sql/hs_standby_disallowed.sql
+++ b/src/test/regress/sql/hs_standby_disallowed.sql
@@ -88,8 +88,6 @@ COMMIT;
 -- Listen
 listen a;
 notify a;
-unlisten a;
-unlisten *;
 
 -- disallowed commands
 
-- 
2.19.1

Tom Lane

tgl@sss.pgh.pa.us

almost 7 years ago

In reply to: Shay Rojansky (#5)

Re: [PATCH] Allow UNLISTEN during recovery

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

Shay Rojansky

roji@roji.org

almost 7 years ago

In reply to: Tom Lane (#6)

Re: [PATCH] Allow UNLISTEN during recovery

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!

Simon Riggs

simon@2ndquadrant.com

almost 7 years ago

In reply to: Tom Lane (#6)

Re: [PATCH] Allow UNLISTEN during recovery

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/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services