doc: modify the comment in function libpqrcv_check_conninfo()

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.

Thank you for your comment!

I've added the must_use_password argument. A new patch is attached.

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.

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 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.

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

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

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

- * 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

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