RE: Fix inappropriate comments in function ReplicationSlotAcquire
On Mon, Jan 29, 2024 at 14:50 Amit Kapila wrote: > Pushed! Thanks for pushing. Regards, Wang Wei
Re: Fix inappropriate comments in function ReplicationSlotAcquire
On Fri, Jan 26, 2024 at 6:47 AM Wei Wang (Fujitsu) wrote: > > It looks good to me. So, I updated the patch as suggested. > Pushed! -- With Regards, Amit Kapila.
RE: Fix inappropriate comments in function ReplicationSlotAcquire
On Thu, Jan 25, 2024 at 20:33 Amit Kapila wrote:
> On Thu, Jan 25, 2024 at 4:01 PM Wei Wang (Fujitsu)
> wrote:
> >
> >
> > Yes, agree. I think these two parts have become slightly outdated after the
> > commit 1632ea4. So also tried to fix the first part of the comment.
> > Attach the new patch.
> >
>
> How about changing it to something simple like:
> diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
> index f2781d0455..84c257a7aa 100644
> --- a/src/backend/replication/slot.c
> +++ b/src/backend/replication/slot.c
> @@ -465,10 +465,7 @@ retry:
>
> LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
>
> - /*
> -* Search for the slot with the specified name if the slot to acquire
> is
> -* not given. If the slot is not found, we either return -1 or
> error out.
> -*/
> +/* Check if the slot exits with the given name. */
> s = SearchNamedReplicationSlot(name, false);
> if (s == NULL || !s->in_use)
> {
It looks good to me. So, I updated the patch as suggested.
Regards,
Wang Wei
v3-0001-Fix-inappropriate-comments-in-function-Replicatio.patch
Description: v3-0001-Fix-inappropriate-comments-in-function-Replicatio.patch
Re: Fix inappropriate comments in function ReplicationSlotAcquire
On Thu, Jan 25, 2024 at 4:01 PM Wei Wang (Fujitsu)
wrote:
>
>
> Yes, agree. I think these two parts have become slightly outdated after the
> commit 1632ea4. So also tried to fix the first part of the comment.
> Attach the new patch.
>
How about changing it to something simple like:
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index f2781d0455..84c257a7aa 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -465,10 +465,7 @@ retry:
LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
- /*
-* Search for the slot with the specified name if the slot to acquire is
-* not given. If the slot is not found, we either return -1 or
error out.
-*/
+/* Check if the slot exits with the given name. */
s = SearchNamedReplicationSlot(name, false);
if (s == NULL || !s->in_use)
{
--
With Regards,
Amit Kapila.
RE: Fix inappropriate comments in function ReplicationSlotAcquire
On Thu, Jan 25, 2024 at 18:39 Amit Kapila wrote:
>
Thanks for your review.
> On Thu, Jan 25, 2024 at 2:57 PM Wei Wang (Fujitsu)
> wrote:
> >
> > In the function ReplicationSlotAcquire(), I found there is a missing in the
> > below comments:
> >
> > ```
> > /*
> > * Search for the slot with the specified name if the slot to
> > acquire is
> > * not given. If the slot is not found, we either return -1 or
> > error out.
> > */
> > s = SearchNamedReplicationSlot(name, false);
> > if (s == NULL || !s->in_use)
> > {
> > LWLockRelease(ReplicationSlotControlLock);
> >
> > ereport(ERROR,
> > (errcode(ERRCODE_UNDEFINED_OBJECT),
> > errmsg("replication slot \"%s\" does not
> exist",
> > name)));
> > }
> > ```
> >
> > It seems that when the slot is not found, we will only error out and will
> > not
> > return -1.
> >
>
> You seem to be correct. However, isn't the first part of the comment
> also slightly confusing? In particular, "... if the slot to acquire is
> not given." In this function, I don't see the case where a slot to
> acquire is given.
Yes, agree. I think these two parts have become slightly outdated after the
commit 1632ea4. So also tried to fix the first part of the comment.
Attach the new patch.
Regards,
Wang Wei
v2-0001-Fix-inappropriate-comments-in-function-Replicatio.patch
Description: v2-0001-Fix-inappropriate-comments-in-function-Replicatio.patch
Re: Fix inappropriate comments in function ReplicationSlotAcquire
On Thu, Jan 25, 2024 at 2:57 PM Wei Wang (Fujitsu)
wrote:
>
> In the function ReplicationSlotAcquire(), I found there is a missing in the
> below comments:
>
> ```
> /*
> * Search for the slot with the specified name if the slot to acquire
> is
> * not given. If the slot is not found, we either return -1 or error
> out.
> */
> s = SearchNamedReplicationSlot(name, false);
> if (s == NULL || !s->in_use)
> {
> LWLockRelease(ReplicationSlotControlLock);
>
> ereport(ERROR,
> (errcode(ERRCODE_UNDEFINED_OBJECT),
> errmsg("replication slot \"%s\" does not
> exist",
> name)));
> }
> ```
>
> It seems that when the slot is not found, we will only error out and will not
> return -1.
>
You seem to be correct. However, isn't the first part of the comment
also slightly confusing? In particular, "... if the slot to acquire is
not given." In this function, I don't see the case where a slot to
acquire is given.
--
With Regards,
Amit Kapila.
