Re: [HACKERS] [PATCH] Fix drop replication slot blocking instead of returning error

[Simone Gotti]

On Fri, Sep 1, 2017 at 4:31 PM, Alvaro Herrera  wrote:
>
> Both patches pushed, which should close the open item.

Hi Alvaro,

thanks a lot!

Cheers,
Simone.


>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Fix drop replication slot blocking instead of returning error

[Alvaro Herrera]

Both patches pushed, which should close the open item.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Fix drop replication slot blocking instead of returning error

[Alvaro Herrera]

And another patch to restore behavior to replication origin drop.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 66c1b1072feb95a08739d9a752f1d6fc73d1dc77 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Wed, 30 Aug 2017 15:38:15 +0200
Subject: [PATCH 2/2] Restore original behavior for replication origins, too

---
 src/backend/replication/logical/origin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/replication/logical/origin.c 
b/src/backend/replication/logical/origin.c
index 14cb3d0bf2..edc6efb8a6 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -1205,7 +1205,7 @@ pg_replication_origin_drop(PG_FUNCTION_ARGS)
roident = replorigin_by_name(name, false);
Assert(OidIsValid(roident));
 
-   replorigin_drop(roident, false);
+   replorigin_drop(roident, true);
 
pfree(name);
 
-- 
2.11.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Fix drop replication slot blocking instead of returning error

[Alvaro Herrera]

Craig Ringer wrote:

> > FWIW, I also don't think it's ok to just change the behaviour
> > unconditionally and without a replacement for existing behaviour.
> 
> Seems like it just needs a new argument nowait DEFAULT false

I added a WAIT flag to DROP_REPLICATION_SLOT, preliminary patch
attached.  Running tests now.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From b395155c504ad1c6a4eb1faa7c74e2df8b559545 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Tue, 29 Aug 2017 12:08:47 +0200
Subject: [PATCH] Add a WAIT option to DROP_REPLICATION_COMMAND

This restores the default behavior of the command prior to 9915de6c1cb2.

Per complaint from Simone Gotti.
Discussion: 
https://postgr.es/m/caevsy6wgdf90o6puvg2wsvxl2omh5opc-38od4zzgk-fxav...@mail.gmail.com
---
 doc/src/sgml/logicaldecoding.sgml   |  2 +-
 doc/src/sgml/protocol.sgml  | 17 ++---
 src/backend/commands/subscriptioncmds.c |  2 +-
 src/backend/replication/repl_gram.y | 10 ++
 src/backend/replication/repl_scanner.l  |  1 +
 src/backend/replication/slotfuncs.c |  2 +-
 src/backend/replication/walsender.c |  2 +-
 src/include/nodes/replnodes.h   |  1 +
 8 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/doc/src/sgml/logicaldecoding.sgml 
b/doc/src/sgml/logicaldecoding.sgml
index 8dcfc6c742..f8142518c1 100644
--- a/doc/src/sgml/logicaldecoding.sgml
+++ b/doc/src/sgml/logicaldecoding.sgml
@@ -303,7 +303,7 @@ $ pg_recvlogical -d postgres --slot test --drop-slot
  
 
  
-  DROP_REPLICATION_SLOT 
slot_name
+  DROP_REPLICATION_SLOT 
slot_name  WAIT 

  
 
  
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 7c012f59a3..2bb4e38a9d 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2173,13 +2173,13 @@ The commands accepted in walsender mode are:
   
 
   
-DROP_REPLICATION_SLOT slot_name
+
+ DROP_REPLICATION_SLOT slot_name  WAIT 
  DROP_REPLICATION_SLOT
 
 
  
-  Drops a replication slot, freeing any reserved server-side resources. If
-  the slot is currently in use by an active connection, this command fails.
+  Drops a replication slot, freeing any reserved server-side resources.
   If the slot is a logical slot that was created in a database other than
   the database the walsender is connected to, this command fails.
  
@@ -2192,6 +2192,17 @@ The commands accepted in walsender mode are:
  

   
+
+  
+   WAIT
+   
+
+ This option causes the command to wait if the slot is active until
+ it becomes inactive, instead of the default behavior of raising an
+ error.
+
+   
+  
  
 
   
diff --git a/src/backend/commands/subscriptioncmds.c 
b/src/backend/commands/subscriptioncmds.c
index 9bc1d178fc..2ef414e084 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -959,7 +959,7 @@ DropSubscription(DropSubscriptionStmt *stmt, bool 
isTopLevel)
load_file("libpqwalreceiver", false);
 
initStringInfo();
-   appendStringInfo(, "DROP_REPLICATION_SLOT %s", 
quote_identifier(slotname));
+   appendStringInfo(, "DROP_REPLICATION_SLOT %s WAIT", 
quote_identifier(slotname));
 
wrconn = walrcv_connect(conninfo, true, subname, );
if (wrconn == NULL)
diff --git a/src/backend/replication/repl_gram.y 
b/src/backend/replication/repl_gram.y
index ec047c827c..a012447fa2 100644
--- a/src/backend/replication/repl_gram.y
+++ b/src/backend/replication/repl_gram.y
@@ -72,6 +72,7 @@ static SQLCmd *make_sqlcmd(void);
 %token K_LABEL
 %token K_PROGRESS
 %token K_FAST
+%token K_WAIT
 %token K_NOWAIT
 %token K_MAX_RATE
 %token K_WAL
@@ -272,6 +273,15 @@ drop_replication_slot:
DropReplicationSlotCmd *cmd;
cmd = makeNode(DropReplicationSlotCmd);
cmd->slotname = $2;
+   cmd->wait = false;
+   $$ = (Node *) cmd;
+   }
+   | K_DROP_REPLICATION_SLOT IDENT K_WAIT
+   {
+   DropReplicationSlotCmd *cmd;
+   cmd = makeNode(DropReplicationSlotCmd);
+   cmd->slotname = $2;
+   cmd->wait = true;
$$ = (Node *) cmd;
}
;
diff --git a/src/backend/replication/repl_scanner.l 
b/src/backend/replication/repl_scanner.l
index 52ae7b343f..62bb5288c0 100644
--- a/src/backend/replication/repl_scanner.l
+++ 

Re: [HACKERS] [PATCH] Fix drop replication slot blocking instead of returning error

[Craig Ringer]

On 29 August 2017 at 22:02, Andres Freund  wrote:

> Hi,
>
> On 2017-08-29 13:42:05 +0200, Simone Gotti wrote:
> > On Tue, Aug 29, 2017 at 12:13 PM, Alvaro Herrera
> >  wrote:
> > >
> >
> > Hi Alvaro,
> >
> > > Simone Gotti wrote:
> > > > Hi all,
> > > >
> > > > I noticed that in postgres 10beta3, calling pg_drop_replication_slot
> on an
> > > > active slot will block until it's released instead of returning an
> error
> > > > like
> > > > done in pg 9.6. Since this is a change in the previous behavior and
> the docs
> > > > wasn't changed I made a patch to restore the previous behavior.
> > >
> > > Changing that behavior was the entire point of the cited commit.
> >
> > Sorry, I was thinking that the new behavior was needed for internal
> > future functions since the doc wasn't changed.
>
> FWIW, I also don't think it's ok to just change the behaviour
> unconditionally and without a replacement for existing behaviour.


Seems like it just needs a new argument nowait DEFAULT false


 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] [PATCH] Fix drop replication slot blocking instead of returning error

[Andres Freund]

Hi,

On 2017-08-29 13:42:05 +0200, Simone Gotti wrote:
> On Tue, Aug 29, 2017 at 12:13 PM, Alvaro Herrera
>  wrote:
> >
> 
> Hi Alvaro,
> 
> > Simone Gotti wrote:
> > > Hi all,
> > >
> > > I noticed that in postgres 10beta3, calling pg_drop_replication_slot on an
> > > active slot will block until it's released instead of returning an error
> > > like
> > > done in pg 9.6. Since this is a change in the previous behavior and the 
> > > docs
> > > wasn't changed I made a patch to restore the previous behavior.
> >
> > Changing that behavior was the entire point of the cited commit.
> 
> Sorry, I was thinking that the new behavior was needed for internal
> future functions since the doc wasn't changed.

FWIW, I also don't think it's ok to just change the behaviour
unconditionally and without a replacement for existing behaviour.


Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Fix drop replication slot blocking instead of returning error

[Simone Gotti]

On Tue, Aug 29, 2017 at 12:13 PM, Alvaro Herrera
 wrote:
>

Hi Alvaro,

> Simone Gotti wrote:
> > Hi all,
> >
> > I noticed that in postgres 10beta3, calling pg_drop_replication_slot on an
> > active slot will block until it's released instead of returning an error
> > like
> > done in pg 9.6. Since this is a change in the previous behavior and the docs
> > wasn't changed I made a patch to restore the previous behavior.
>
> Changing that behavior was the entire point of the cited commit.

Sorry, I was thinking that the new behavior was needed for internal
future functions since the doc wasn't changed.

>
> A better fix, from my perspective, is to amend the docs as per the
> attached patch.  This is what would be useful for logical replication,
> which is what replication slots were invented for in the first place.

I don't know the reasons why the new behavior is better for logical
replication so I trust you. We are using repl slots for physical
replication.

> If you disagree, let's discuss what other use cases you have, and we can
> come up with alternatives that satisfy both.

I just faced the opposite problem, in stolon [1], we currently rely on
the previous behavior. i.e. we don't want to block waiting for a slot
to be released (that under some partitioning conditions could not
happen for a long time), but prefer to continue retrying the drop
later. Now we partially avoid blocking timing out the drop operation
after some seconds.
Another idea will be to query the slot status before doing the drop
but will lead to a race condition (probably the opposite that that
commit was fixing) if the slot is acquired between the query and the
drop.

> I think a decent answer,
> but one which would create a bit of extra churn, would be to have an
> optional boolean flag in the command/function for "nowait", instead of
> hardcoding either behavior.

I think that this will be the best fix. I'm not sure on the policy of
these commands and if backward compatibility will be better (in such
case the old behavior should be the default and a new "wait" flag
could be added).

If the default behavior is going to change we have to add different
code for postgres >= 10.


Thanks,
Simone.


[1] https://github.com/sorintlab/stolon

>
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Fix drop replication slot blocking instead of returning error

[Alvaro Herrera]

Simone Gotti wrote:
> Hi all,
> 
> I noticed that in postgres 10beta3, calling pg_drop_replication_slot on an
> active slot will block until it's released instead of returning an error
> like
> done in pg 9.6. Since this is a change in the previous behavior and the docs
> wasn't changed I made a patch to restore the previous behavior.

Changing that behavior was the entire point of the cited commit.

A better fix, from my perspective, is to amend the docs as per the
attached patch.  This is what would be useful for logical replication,
which is what replication slots were invented for in the first place.
If you disagree, let's discuss what other use cases you have, and we can
come up with alternatives that satisfy both.  I think a decent answer,
but one which would create a bit of extra churn, would be to have an
optional boolean flag in the command/function for "nowait", instead of
hardcoding either behavior.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 5761b79c6ebc4f6dfaf4d2489e9bff47b9e0c3ad Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Tue, 29 Aug 2017 12:08:47 +0200
Subject: [PATCH] Document that DROP_REPLICATION_SLOT now waits

---
 doc/src/sgml/protocol.sgml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 7c012f59a3..e61d57a234 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2179,7 +2179,8 @@ The commands accepted in walsender mode are:
 
  
   Drops a replication slot, freeing any reserved server-side resources. If
-  the slot is currently in use by an active connection, this command fails.
+  the slot is currently in use by an active connection, this command waits
+  until the slot becomes free.
   If the slot is a logical slot that was created in a database other than
   the database the walsender is connected to, this command fails.
  
-- 
2.11.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] [PATCH] Fix drop replication slot blocking instead of returning error

[Simone Gotti]

Hi all,

I noticed that in postgres 10beta3, calling pg_drop_replication_slot on an
active slot will block until it's released instead of returning an error
like
done in pg 9.6. Since this is a change in the previous behavior and the docs
wasn't changed I made a patch to restore the previous behavior.

Thanks,

Simone.

--

after git commit 9915de6c1cb calls to pg_drop_replication_slot or the
replication protocol DROP_REPLICATION_SLOT command will block when a
slot is in use instead of returning an error as before (as the doc
states).

This patch will set nowait to true to restore the previous
behavior.
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index d4cbd83bde..ab776e85d2 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -171,7 +171,7 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS)
 
 	CheckSlotRequirements();
 
-	ReplicationSlotDrop(NameStr(*name), false);
+	ReplicationSlotDrop(NameStr(*name), true);
 
 	PG_RETURN_VOID();
 }
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 03e1cf44de..c6b40ec0fb 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1028,7 +1028,7 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
 static void
 DropReplicationSlot(DropReplicationSlotCmd *cmd)
 {
-	ReplicationSlotDrop(cmd->slotname, false);
+	ReplicationSlotDrop(cmd->slotname, true);
 	EndCommand("DROP_REPLICATION_SLOT", DestRemote);
 }
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers