Horton

Thread

Commits

  1. Fix comment in libpqrcv_check_conninfo().

  1. doc: modify the comment in function libpqrcv_check_conninfo()

    Rintaro Ikeda <ikedarintarof@oss.nttdata.com> — 2024-06-26T12:52:55Z

    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

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

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-06-26T14:36:49Z

    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.

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

    Rintaro Ikeda <ikedarintarof@oss.nttdata.com> — 2024-06-27T07:09:37Z

    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.

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

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-06-27T08:18:39Z

    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.

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

    Rintaro Ikeda <ikedarintarof@oss.nttdata.com> — 2024-06-27T10:27:33Z

    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.

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

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-07-01T09:15:14Z

    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.

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-07-08T06:28:04Z

    
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

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

    Rintaro Ikeda <ikedarintarof@oss.nttdata.com> — 2024-07-08T11:44:56Z

    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

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-07-09T00:24:48Z

    
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

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

    Rintaro Ikeda <ikedarintarof@oss.nttdata.com> — 2024-07-09T06:56:34Z

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

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-07-09T12:33:33Z

    
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