pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feeddoc: modify the comment in function libpqrcv_check_conninfo()
11+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2024-07-09 629520be5f Fix comment in libpqrcv_check_conninfo().
* doc: modify the comment in function libpqrcv_check_conninfo()
@ 2024-06-26 12:52 ikedarintarof <ikedarintarof@oss.nttdata.com>
0 siblings, 1 reply; 11+ messages in thread
From: ikedarintarof @ 2024-06-26 12:52 UTC (permalink / raw)
To: pgsql-hackers
Hi,
The function 'libpqrcv_check_conninfo()' returns 'void', but the comment
above says that the function returns true or false.
I've attached a patch to modify the comment.
Regard,
Rintaro Ikeda
Attachments:
mod-doc.patch (text/x-diff, 810B) download
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 02f12f2921..851bf44fd3 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -309,8 +309,8 @@ bad_connection:
* local filesystem access to be attempted.
*
* If the connection string can't be parsed, this function will raise
- * an error and will not return. If it can, it will return true if this
- * connection string specifies a password and false otherwise.
+ * an error. If it can, it will do nothing if this connection string
+ * specifies a password and raise an error otherwise.
*/
static void
libpqrcv_check_conninfo(const char *conninfo, bool must_use_password)
* Re: doc: modify the comment in function libpqrcv_check_conninfo()
@ 2024-06-26 14:36 Jelte Fennema-Nio <postgres@jeltef.nl>
parent: ikedarintarof <ikedarintarof@oss.nttdata.com>
0 siblings, 1 reply; 11+ messages in thread
From: Jelte Fennema-Nio @ 2024-06-26 14:36 UTC (permalink / raw)
To: ikedarintarof <ikedarintarof@oss.nttdata.com>; +Cc: pgsql-hackers
On Wed, 26 Jun 2024 at 14:53, ikedarintarof
<ikedarintarof@oss.nttdata.com> wrote:
> The function 'libpqrcv_check_conninfo()' returns 'void', but the comment
> above says that the function returns true or false.
> I've attached a patch to modify the comment.
Agreed that the current comment is wrong, but the new comment should
mention the must_use_password argument. Because only if
must_use_password is true, will it throw an error.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: doc: modify the comment in function libpqrcv_check_conninfo()
@ 2024-06-27 07:09 ikedarintarof <ikedarintarof@oss.nttdata.com>
parent: Jelte Fennema-Nio <postgres@jeltef.nl>
0 siblings, 1 reply; 11+ messages in thread
From: ikedarintarof @ 2024-06-27 07:09 UTC (permalink / raw)
To: Jelte Fennema-Nio <postgres@jeltef.nl>; +Cc: pgsql-hackers
Thank you for your comment!
I've added the must_use_password argument. A new patch is attached.
On 2024-06-26 23:36, Jelte Fennema-Nio wrote:
> On Wed, 26 Jun 2024 at 14:53, ikedarintarof
> <ikedarintarof@oss.nttdata.com> wrote:
>> The function 'libpqrcv_check_conninfo()' returns 'void', but the
>> comment
>> above says that the function returns true or false.
>> I've attached a patch to modify the comment.
>
> Agreed that the current comment is wrong, but the new comment should
> mention the must_use_password argument. Because only if
> must_use_password is true, will it throw an error.
Attachments:
0001-modify-the-commnet-in-libpqrcv_check_conninfo.patch (text/x-diff, 1.3K) download
From 18c302625176a38ecf95ede0bd0e5a4104358823 Mon Sep 17 00:00:00 2001
From: Rintaro Ikeda <51394766+rinikeda@users.noreply.github.com>
Date: Thu, 27 Jun 2024 16:04:08 +0900
Subject: [PATCH] modify the commnet in libpqrcv_check_conninfo()
---
src/backend/replication/libpqwalreceiver/libpqwalreceiver.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 02f12f2921..252e97a19b 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -309,8 +309,9 @@ bad_connection:
* local filesystem access to be attempted.
*
* If the connection string can't be parsed, this function will raise
- * an error and will not return. If it can, it will return true if this
- * connection string specifies a password and false otherwise.
+ * an error. If it can and must_use_password is false, it will do nothing.
+ * If must_use_password is true, the function checks whther a password is
+ * provided and decides an error should be raised.
*/
static void
libpqrcv_check_conninfo(const char *conninfo, bool must_use_password)
--
2.39.3 (Apple Git-146)
* Re: doc: modify the comment in function libpqrcv_check_conninfo()
@ 2024-06-27 08:18 Jelte Fennema-Nio <postgres@jeltef.nl>
parent: ikedarintarof <ikedarintarof@oss.nttdata.com>
0 siblings, 1 reply; 11+ messages in thread
From: Jelte Fennema-Nio @ 2024-06-27 08:18 UTC (permalink / raw)
To: ikedarintarof <ikedarintarof@oss.nttdata.com>; +Cc: pgsql-hackers
On Thu, 27 Jun 2024 at 09:09, ikedarintarof
<ikedarintarof@oss.nttdata.com> wrote:
>
> Thank you for your comment!
>
> I've added the must_use_password argument. A new patch is attached.
s/whther/whether
nit: "it will do nothing" sounds a bit strange to me (but I'm not
native english). Something like this reads more natural to me:
an error. If must_use_password is true, the function raises an error
if no password is provided in the connection string. In any other case
it successfully completes.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: doc: modify the comment in function libpqrcv_check_conninfo()
@ 2024-06-27 10:27 ikedarintarof <ikedarintarof@oss.nttdata.com>
parent: Jelte Fennema-Nio <postgres@jeltef.nl>
0 siblings, 1 reply; 11+ messages in thread
From: ikedarintarof @ 2024-06-27 10:27 UTC (permalink / raw)
To: Jelte Fennema-Nio <postgres@jeltef.nl>; +Cc: pgsql-hackers
Thanks for your suggestion. I used ChatGPT to choose the wording, but
it's still difficult for me.
The new patch includes your suggestion.
On 2024-06-27 17:18, Jelte Fennema-Nio wrote:
> On Thu, 27 Jun 2024 at 09:09, ikedarintarof
> <ikedarintarof@oss.nttdata.com> wrote:
>>
>> Thank you for your comment!
>>
>> I've added the must_use_password argument. A new patch is attached.
>
> s/whther/whether
>
> nit: "it will do nothing" sounds a bit strange to me (but I'm not
> native english). Something like this reads more natural to me:
>
> an error. If must_use_password is true, the function raises an error
> if no password is provided in the connection string. In any other case
> it successfully completes.
Attachments:
0001-modify-the-commnet-in-libpqrcv_check_conninfo.patch (text/x-diff, 1.2K) download
From c01cadcb955d79ff525992985bd59736c6731abe Mon Sep 17 00:00:00 2001
From: Rintaro Ikeda <51394766+rinikeda@users.noreply.github.com>
Date: Thu, 27 Jun 2024 16:04:08 +0900
Subject: [PATCH] modify the commnet in libpqrcv_check_conninfo()
---
src/backend/replication/libpqwalreceiver/libpqwalreceiver.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 02f12f2921..4f7b69a73d 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -309,8 +309,9 @@ bad_connection:
* local filesystem access to be attempted.
*
* If the connection string can't be parsed, this function will raise
- * an error and will not return. If it can, it will return true if this
- * connection string specifies a password and false otherwise.
+ * an error. If must_use_password is true, the function raises an error
+ * if no password is provided in the connection string. In any other case
+ * it successfully completes.
*/
static void
libpqrcv_check_conninfo(const char *conninfo, bool must_use_password)
--
2.39.3 (Apple Git-146)
* Re: doc: modify the comment in function libpqrcv_check_conninfo()
@ 2024-07-01 09:15 Jelte Fennema-Nio <postgres@jeltef.nl>
parent: ikedarintarof <ikedarintarof@oss.nttdata.com>
0 siblings, 1 reply; 11+ messages in thread
From: Jelte Fennema-Nio @ 2024-07-01 09:15 UTC (permalink / raw)
To: ikedarintarof <ikedarintarof@oss.nttdata.com>; Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers
On Thu, 27 Jun 2024 at 12:27, ikedarintarof
<ikedarintarof@oss.nttdata.com> wrote:
> Thanks for your suggestion. I used ChatGPT to choose the wording, but
> it's still difficult for me.
Looks good to me now (but obviously biased since you took my wording).
Adding Robert, since he authored the commit that introduced this
comment.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: doc: modify the comment in function libpqrcv_check_conninfo()
@ 2024-07-08 06:28 Fujii Masao <masao.fujii@oss.nttdata.com>
parent: Jelte Fennema-Nio <postgres@jeltef.nl>
0 siblings, 1 reply; 11+ messages in thread
From: Fujii Masao @ 2024-07-08 06:28 UTC (permalink / raw)
To: Jelte Fennema-Nio <postgres@jeltef.nl>; ikedarintarof <ikedarintarof@oss.nttdata.com>; Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers
On 2024/07/01 18:15, Jelte Fennema-Nio wrote:
> On Thu, 27 Jun 2024 at 12:27, ikedarintarof
> <ikedarintarof@oss.nttdata.com> wrote:
>> Thanks for your suggestion. I used ChatGPT to choose the wording, but
>> it's still difficult for me.
>
> Looks good to me now (but obviously biased since you took my wording).
LGTM, too.
* Validate connection info string, and determine whether it might cause
* local filesystem access to be attempted.
The later part of the above comment for libpqrcv_check_conninfo()
seems unclear. My understanding is that if must_use_password is true
and this function completes without error, the password must be
in the connection string, so there's no need to read the .pgpass file
(i.e., no local filesystem access). That part seems to be trying to
explaing something like that. Is this correct?
Anyway, wouldn't it be better to either remove this unclear part or
rephrase it for clarity?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: doc: modify the comment in function libpqrcv_check_conninfo()
@ 2024-07-08 11:44 ikedarintarof <ikedarintarof@oss.nttdata.com>
parent: Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 1 reply; 11+ messages in thread
From: ikedarintarof @ 2024-07-08 11:44 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: Jelte Fennema-Nio <postgres@jeltef.nl>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On 2024-07-08 15:28, Fujii Masao wrote:
> On 2024/07/01 18:15, Jelte Fennema-Nio wrote:
>> On Thu, 27 Jun 2024 at 12:27, ikedarintarof
>> <ikedarintarof@oss.nttdata.com> wrote:
>>> Thanks for your suggestion. I used ChatGPT to choose the wording, but
>>> it's still difficult for me.
>>
>> Looks good to me now (but obviously biased since you took my wording).
>
> LGTM, too.
>
>
> * Validate connection info string, and determine whether it might
> cause
> * local filesystem access to be attempted.
>
> The later part of the above comment for libpqrcv_check_conninfo()
> seems unclear. My understanding is that if must_use_password is true
> and this function completes without error, the password must be
> in the connection string, so there's no need to read the .pgpass file
> (i.e., no local filesystem access). That part seems to be trying to
> explaing something like that. Is this correct?
>
> Anyway, wouldn't it be better to either remove this unclear part or
> rephrase it for clarity?
>
> Regards,
Thank you for your comment.
I agree "local filesystem access" is vague. I think the reference to
.pgpass
(local file system) is not necessary in the comment for
libpqrcv_check_conninfo()
because it is explained in libpqrcv_connect() as shown below.
> /*
> * Re-validate connection string. The validation already happened at DDL
> * time, but the subscription owner may have changed. If we don't
> recheck
> * with the correct must_use_password, it's possible that the connection
> * will obtain the password from a different source, such as PGPASSFILE
> or
> * PGPASSWORD.
> */
> libpqrcv_check_conninfo(conninfo, must_use_password);
I remove the unclear part from the previous patch and add some
explanation for
later part of the function.
Or, I think it is also good to make the comment more simple:
> * The function checks that
> * 1. connection info string is properly parsed and
> * 2. a password is provided if must_use_password is true.
> * If the check is failed, the it will raise an error.
> */
> static void
> libpqrcv_check_conninfo(const char *conninfo, bool must_use_password)
Regards,
Rintaro Ikeda
Attachments:
v2-0001-modify-the-commnet-in-libpqrcv_check_conninfo.patch (text/x-diff, 1.4K) download
From 514bd81f7d1b7104e6d2d92cb072a84096cd3885 Mon Sep 17 00:00:00 2001
From: Rintaro Ikeda <51394766+rinikeda@users.noreply.github.com>
Date: Thu, 27 Jun 2024 16:04:08 +0900
Subject: [PATCH v2] modify the commnet in libpqrcv_check_conninfo()
---
.../replication/libpqwalreceiver/libpqwalreceiver.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 02f12f2921..e6a4e4cc1c 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -305,12 +305,14 @@ bad_connection:
}
/*
- * Validate connection info string, and determine whether it might cause
- * local filesystem access to be attempted.
+ * The function
+ * 1. validates connection info string and
+ * 2. checks a password is provided if must_use_password is true.
*
* If the connection string can't be parsed, this function will raise
- * an error and will not return. If it can, it will return true if this
- * connection string specifies a password and false otherwise.
+ * an error. If must_use_password is true, the function raises an error
+ * if no password is provided in the connection string. In any other case
+ * it successfully completes.
*/
static void
libpqrcv_check_conninfo(const char *conninfo, bool must_use_password)
--
2.39.3 (Apple Git-146)
* Re: doc: modify the comment in function libpqrcv_check_conninfo()
@ 2024-07-09 00:24 Fujii Masao <masao.fujii@oss.nttdata.com>
parent: ikedarintarof <ikedarintarof@oss.nttdata.com>
0 siblings, 1 reply; 11+ messages in thread
From: Fujii Masao @ 2024-07-09 00:24 UTC (permalink / raw)
To: ikedarintarof <ikedarintarof@oss.nttdata.com>; +Cc: Jelte Fennema-Nio <postgres@jeltef.nl>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On 2024/07/08 20:44, ikedarintarof wrote:
> I remove the unclear part from the previous patch and add some explanation for
> later part of the function.
- * Validate connection info string, and determine whether it might cause
- * local filesystem access to be attempted.
+ * The function
+ * 1. validates connection info string and
+ * 2. checks a password is provided if must_use_password is true.
IMO, "Validate connection info string" is sufficient. The mention of
must_use_password is redundant since it's explained in the latter part of
the comment. Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: doc: modify the comment in function libpqrcv_check_conninfo()
@ 2024-07-09 06:56 ikedarintarof <ikedarintarof@oss.nttdata.com>
parent: Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 1 reply; 11+ messages in thread
From: ikedarintarof @ 2024-07-09 06:56 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: Jelte Fennema-Nio <postgres@jeltef.nl>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
> - * Validate connection info string, and determine whether it might
> cause
> - * local filesystem access to be attempted.
> + * The function
> + * 1. validates connection info string and
> + * 2. checks a password is provided if must_use_password is true.
>
> IMO, "Validate connection info string" is sufficient. The mention of
> must_use_password is redundant since it's explained in the latter part
> of
> the comment.
That is reasonable for me. I've removed the second one.
The modified patch is attached.
Regards,
Rintaro Ikeda
Attachments:
v4-0001-modify-the-commnet-in-libpqrcv_check_conninfo.patch (text/x-diff, 1.4K) download
From 7a88ad7f306519648ad98eaf03e501e66ba9d5ea Mon Sep 17 00:00:00 2001
From: Rintaro Ikeda <51394766+rinikeda@users.noreply.github.com>
Date: Thu, 27 Jun 2024 16:04:08 +0900
Subject: [PATCH v4] modify the commnet in libpqrcv_check_conninfo()
---
.../replication/libpqwalreceiver/libpqwalreceiver.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 02f12f2921..6c42c209d2 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -305,12 +305,12 @@ bad_connection:
}
/*
- * Validate connection info string, and determine whether it might cause
- * local filesystem access to be attempted.
+ * Validate connection info string.
*
* If the connection string can't be parsed, this function will raise
- * an error and will not return. If it can, it will return true if this
- * connection string specifies a password and false otherwise.
+ * an error. If must_use_password is true, the function raises an error
+ * if no password is provided in the connection string. In any other case
+ * it successfully completes.
*/
static void
libpqrcv_check_conninfo(const char *conninfo, bool must_use_password)
--
2.39.3 (Apple Git-146)
* Re: doc: modify the comment in function libpqrcv_check_conninfo()
@ 2024-07-09 12:33 Fujii Masao <masao.fujii@oss.nttdata.com>
parent: ikedarintarof <ikedarintarof@oss.nttdata.com>
0 siblings, 0 replies; 11+ messages in thread
From: Fujii Masao @ 2024-07-09 12:33 UTC (permalink / raw)
To: ikedarintarof <ikedarintarof@oss.nttdata.com>; +Cc: Jelte Fennema-Nio <postgres@jeltef.nl>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On 2024/07/09 15:56, ikedarintarof wrote:
>> - * Validate connection info string, and determine whether it might cause
>> - * local filesystem access to be attempted.
>> + * The function
>> + * 1. validates connection info string and
>> + * 2. checks a password is provided if must_use_password is true.
>>
>> IMO, "Validate connection info string" is sufficient. The mention of
>> must_use_password is redundant since it's explained in the latter part of
>> the comment.
>
> That is reasonable for me. I've removed the second one.
> The modified patch is attached.
Thanks for updating the patch! Pushed.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2024-07-09 12:33 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-06-26 12:52 doc: modify the comment in function libpqrcv_check_conninfo() ikedarintarof <ikedarintarof@oss.nttdata.com>
2024-06-26 14:36 ` Jelte Fennema-Nio <postgres@jeltef.nl>
2024-06-27 07:09 ` ikedarintarof <ikedarintarof@oss.nttdata.com>
2024-06-27 08:18 ` Jelte Fennema-Nio <postgres@jeltef.nl>
2024-06-27 10:27 ` ikedarintarof <ikedarintarof@oss.nttdata.com>
2024-07-01 09:15 ` Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-08 06:28 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 11:44 ` ikedarintarof <ikedarintarof@oss.nttdata.com>
2024-07-09 00:24 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-09 06:56 ` ikedarintarof <ikedarintarof@oss.nttdata.com>
2024-07-09 12:33 ` Fujii Masao <masao.fujii@oss.nttdata.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox