Re: doc: modify the comment in function libpqrcv_check_conninfo()

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