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
