Fix inappropriate comments in function ReplicationSlotAcquire
Hi,
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. Attach the patch to remove the inappropriate description of return
value.
Regards,
Wang Wei
Attachments:
v1-0001-Fix-inappropriate-comments-in-function-Replicatio.patchapplication/octet-stream; name=v1-0001-Fix-inappropriate-comments-in-function-Replicatio.patchDownload
From cda6311a27688120dd21dfaad4edb7fed3f455f7 Mon Sep 17 00:00:00 2001
From: Wang Wei <wangw.fnst@fujitsu.com>
Date: Thu, 25 Jan 2024 17:04:41 +0900
Subject: [PATCH v1] Fix inappropriate comments in function
ReplicationSlotAcquire
Fix the inappropriate description of return value in function
ReplicationSlotAcquire. We will only error out and will not return -1.
---
src/backend/replication/slot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 02a14ec210..fee692dbbf 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -467,7 +467,7 @@ retry:
/*
* 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.
+ * not given. Error out if the slot is not found.
*/
s = SearchNamedReplicationSlot(name, false);
if (s == NULL || !s->in_use)
--
2.39.1.windows.1
On Thu, Jan 25, 2024 at 2:57 PM Wei Wang (Fujitsu)
<wangw.fnst@fujitsu.com> 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.
On Thu, Jan 25, 2024 at 18:39 Amit Kapila <amit.kapila16@gmail.com> wrote:
Thanks for your review.
On Thu, Jan 25, 2024 at 2:57 PM Wei Wang (Fujitsu)
<wangw.fnst@fujitsu.com> 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 notexist",
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
Attachments:
v2-0001-Fix-inappropriate-comments-in-function-Replicatio.patchapplication/octet-stream; name=v2-0001-Fix-inappropriate-comments-in-function-Replicatio.patchDownload
From c5289dc5c3a3c2b620f8c5e2a0bb8214caa6b1b7 Mon Sep 17 00:00:00 2001
From: Wang Wei <wangw.fnst@fujitsu.com>
Date: Thu, 25 Jan 2024 17:04:41 +0900
Subject: [PATCH v2] Fix inappropriate comments in function
ReplicationSlotAcquire
Remove the inappropriate description of return value in function
ReplicationSlotAcquire. We will only error out and will not return -1.
---
src/backend/replication/slot.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 02a14ec210..62c08bc1b5 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -466,8 +466,8 @@ 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.
+ * Search for the slot with the specified name. Error out if the slot is
+ * not found.
*/
s = SearchNamedReplicationSlot(name, false);
if (s == NULL || !s->in_use)
--
2.39.1.windows.1
On Thu, Jan 25, 2024 at 4:01 PM Wei Wang (Fujitsu)
<wangw.fnst@fujitsu.com> 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.
On Thu, Jan 25, 2024 at 20:33 Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jan 25, 2024 at 4:01 PM Wei Wang (Fujitsu)
<wangw.fnst@fujitsu.com> 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
Attachments:
v3-0001-Fix-inappropriate-comments-in-function-Replicatio.patchapplication/octet-stream; name=v3-0001-Fix-inappropriate-comments-in-function-Replicatio.patchDownload
From 8667fc36dab8276f61aaa4996d938eda0269b9ec Mon Sep 17 00:00:00 2001
From: Wang Wei <wangw.fnst@fujitsu.com>
Date: Thu, 25 Jan 2024 17:04:41 +0900
Subject: [PATCH v3] Fix inappropriate comments in function
ReplicationSlotAcquire
The comment have become slightly outdated after the commit 1632ea4.
Improve it.
---
src/backend/replication/slot.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 02a14ec210..5e3ec0b666 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)
{
--
2.39.1.windows.1
On Fri, Jan 26, 2024 at 6:47 AM Wei Wang (Fujitsu)
<wangw.fnst@fujitsu.com> wrote:
It looks good to me. So, I updated the patch as suggested.
Pushed!
--
With Regards,
Amit Kapila.