From: ikedarintarof <ikedarintarof@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Jelte Fennema-Nio <postgres@jeltef.nl>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: doc: modify the comment in function libpqrcv_check_conninfo()
Date: Mon, 08 Jul 2024 20:44:56 +0900
Message-ID: <d5f5fb21f27c17c400a4dbe386d008a8@oss.nttdata.com> (raw)
In-Reply-To: <202ec279-c307-48b9-9920-730fb1221d27@oss.nttdata.com>
References: <6a1ca81b27fec4da0ccdfaaaec787982@oss.nttdata.com>
<CAGECzQRjJkCQ9AUQA_7kJw8c4iUwVAXPfD_VQu+-DVcd-LCT8A@mail.gmail.com>
<9c15a4b74d09348d03b02934fc429c94@oss.nttdata.com>
<CAGECzQQ+bpYVuF6GpVdc9vzHOObs5HJLmj5YohOb2d-ctmCffg@mail.gmail.com>
<c380192faada009af9aa5bfcd06e140d@oss.nttdata.com>
<CAGECzQRPygEJr3Ed2p7QsUsBkecUa0mpiE3zMOEh5_NtvAfgRw@mail.gmail.com>
<202ec279-c307-48b9-9920-730fb1221d27@oss.nttdata.com>
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.cindex 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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ikedarintarof@oss.nttdata.com, masao.fujii@oss.nttdata.com, postgres@jeltef.nl, robertmhaas@gmail.com
Subject: Re: doc: modify the comment in function libpqrcv_check_conninfo()
In-Reply-To: <d5f5fb21f27c17c400a4dbe386d008a8@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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