Re: Review for GetWALAvailability() - Mailing list pgsql-hackers

At Wed, 17 Jun 2020 20:13:01 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> > ReplicationSlotAcquireInternal.  I think we should call
> > ConditionVariablePrepareToSleep before the sorrounding for statement
> > block.
> 
> OK, so what about the attached patch? I added
> ConditionVariablePrepareToSleep()
> just before entering the "for" loop in
> InvalidateObsoleteReplicationSlots().

Thanks.

ReplicationSlotAcquireInternal:
+ * If *slot == NULL, search for the slot with the given name.

'*' seems needless here.


The patch moves ConditionVariablePrepareToSleep. We need to call the
function before looking into active_pid as originally commented.
Since it is not protected by ReplicationSlotControLock, just before
releasing the lock is not correct.

The attached on top of the v3 fixes that.



+   s = (slot == NULL) ? SearchNamedReplicationSlot(name) : slot;
+   if (s == NULL || !s->in_use || strcmp(name, NameStr(s->data.name)) != 0)

The conditions in the second line is needed for the case slot is
given, but it is already done in SearchNamedReplicationSlot if slot is
not given.  I would like something like the following instead, but I
don't insist on it.

    ReplicationSlot *s = NULL;
    ...
    if (!slot)
        s = SearchNamedReplicationSlot(name);
    else if(s->in_use && strcmp(name, NameStr(s->data.name)))
        s = slot;


+        ereport(ERROR,
+                (errcode(ERRCODE_UNDEFINED_OBJECT),
+                 errmsg("replication slot \"%s\" does not exist", name)));

The error message is not right when the given slot doesn't match the
given name.  It might be better to leave it to the caller.  Currently
no such caller exists so I don't insist on this but the message should
be revised otherwise.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 8f1913ec7ff3809d11adf1611aba57e44cb42a82 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Thu, 18 Jun 2020 10:55:49 +0900
Subject: [PATCH 1/3] 001

---
 src/backend/replication/slot.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index b94e11a8e7..dcc76c4783 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -412,6 +412,14 @@ retry:
      */
     if (IsUnderPostmaster)
     {
+        /*
+         * Get ready to sleep on it in case it is active if needed.
+         * (We may end up not sleeping, but we don't want to do this while
+         * holding the spinlock.)
+         */
+        if (behavior != SAB_Inquire)
+            ConditionVariablePrepareToSleep(&s->active_cv);
+
         SpinLockAcquire(&s->mutex);
         if (s->active_pid == 0)
             s->active_pid = MyProcPid;
@@ -438,12 +446,13 @@ retry:
             return active_pid;
 
         /* Wait here until we get signaled, and then restart */
-        ConditionVariablePrepareToSleep(&s->active_cv);
         ConditionVariableSleep(&s->active_cv,
                                WAIT_EVENT_REPLICATION_SLOT_DROP);
         ConditionVariableCancelSleep();
         goto retry;
     }
+    else if (behavior != SAB_Inquire)
+        ConditionVariableCancelSleep();
 
     /* Let everybody know we've modified this slot */
     ConditionVariableBroadcast(&s->active_cv);
-- 
2.18.4

From 1b2f94d6bfb4508b2cbf3d552a5615ae2959e90c Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Thu, 18 Jun 2020 11:25:25 +0900
Subject: [PATCH 2/3] 002

---
 src/backend/replication/slot.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index dcc76c4783..8893516f00 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -380,7 +380,7 @@ static int
 ReplicationSlotAcquireInternal(ReplicationSlot *slot, const char *name,
                                SlotAcquireBehavior behavior)
 {
-    ReplicationSlot *s;
+    ReplicationSlot *s = NULL;
     int            active_pid;
 
 retry:
@@ -393,8 +393,12 @@ retry:
      * acquire is not given. If the slot is not found, we either
      * return -1 or error out.
      */
-    s = (slot == NULL) ? SearchNamedReplicationSlot(name) : slot;
-    if (s == NULL || !s->in_use || strcmp(name, NameStr(s->data.name)) != 0)
+    if (!slot)
+        s = SearchNamedReplicationSlot(name);
+    else if(s->in_use && strcmp(name, NameStr(s->data.name)))
+        s = slot;
+
+    if (s == NULL)
     {
         if (behavior == SAB_Inquire)
         {
-- 
2.18.4

From 2d4a83abd2f278a9f9dc6e9329aed1acc191f2c8 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Thu, 18 Jun 2020 11:33:29 +0900
Subject: [PATCH 3/3] 003

---
 src/backend/replication/slot.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 8893516f00..25ae334a29 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -373,8 +373,10 @@ ReplicationSlotAcquire(const char *name, SlotAcquireBehavior behavior)
  * Mark the specified slot as used by this process.
  *
  * If *slot == NULL, search for the slot with the given name.
- *
  * See comments about the return value in ReplicationSlotAcquire().
+ *
+ * If slot is not NULL, returns -1 if the slot is not in use or doesn't match
+ * the given name.
  */
 static int
 ReplicationSlotAcquireInternal(ReplicationSlot *slot, const char *name,
@@ -400,7 +402,7 @@ retry:
 
     if (s == NULL)
     {
-        if (behavior == SAB_Inquire)
+        if (behavior == SAB_Inquire || slot)
         {
             LWLockRelease(ReplicationSlotControlLock);
             return -1;
-- 
2.18.4