Re: Review for GetWALAvailability()

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