MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_20e8bc327f30acfcd0754f348d90586a"
Content-Transfer-Encoding: 7bit
Date: Mon, 08 Jul 2024 20:44:56 +0900
From: ikedarintarof <ikedarintarof@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Jelte Fennema-Nio <postgres@jeltef.nl>, Robert Haas
 <robertmhaas@gmail.com>, pgsql-hackers@postgresql.org
Subject: Re: doc: modify the comment in function libpqrcv_check_conninfo()
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>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <d5f5fb21f27c17c400a4dbe386d008a8@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/d5f5fb21f27c17c400a4dbe386d008a8%40oss.nttdata.com>
Precedence: bulk

--=_20e8bc327f30acfcd0754f348d90586a
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=US-ASCII;
 format=flowed

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
--=_20e8bc327f30acfcd0754f348d90586a
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
 name=v2-0001-modify-the-commnet-in-libpqrcv_check_conninfo.patch
Content-Disposition: attachment;
 filename=v2-0001-modify-the-commnet-in-libpqrcv_check_conninfo.patch;
 size=1474

RnJvbSA1MTRiZDgxZjdkMWI3MTA0ZTZkMmQ5MmNiMDcyYTg0MDk2Y2QzODg1IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBSaW50YXJvIElrZWRhIDw1MTM5NDc2NityaW5pa2VkYUB1c2Vy
cy5ub3JlcGx5LmdpdGh1Yi5jb20+CkRhdGU6IFRodSwgMjcgSnVuIDIwMjQgMTY6MDQ6MDggKzA5
MDAKU3ViamVjdDogW1BBVENIIHYyXSBtb2RpZnkgdGhlIGNvbW1uZXQgaW4gbGlicHFyY3ZfY2hl
Y2tfY29ubmluZm8oKQoKLS0tCiAuLi4vcmVwbGljYXRpb24vbGlicHF3YWxyZWNlaXZlci9saWJw
cXdhbHJlY2VpdmVyLmMgICAgfCAxMCArKysrKystLS0tCiAxIGZpbGUgY2hhbmdlZCwgNiBpbnNl
cnRpb25zKCspLCA0IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxp
Y2F0aW9uL2xpYnBxd2FscmVjZWl2ZXIvbGlicHF3YWxyZWNlaXZlci5jIGIvc3JjL2JhY2tlbmQv
cmVwbGljYXRpb24vbGlicHF3YWxyZWNlaXZlci9saWJwcXdhbHJlY2VpdmVyLmMKaW5kZXggMDJm
MTJmMjkyMS4uZTZhNGU0Y2MxYyAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24v
bGlicHF3YWxyZWNlaXZlci9saWJwcXdhbHJlY2VpdmVyLmMKKysrIGIvc3JjL2JhY2tlbmQvcmVw
bGljYXRpb24vbGlicHF3YWxyZWNlaXZlci9saWJwcXdhbHJlY2VpdmVyLmMKQEAgLTMwNSwxMiAr
MzA1LDE0IEBAIGJhZF9jb25uZWN0aW9uOgogfQogCiAvKgotICogVmFsaWRhdGUgY29ubmVjdGlv
biBpbmZvIHN0cmluZywgYW5kIGRldGVybWluZSB3aGV0aGVyIGl0IG1pZ2h0IGNhdXNlCi0gKiBs
b2NhbCBmaWxlc3lzdGVtIGFjY2VzcyB0byBiZSBhdHRlbXB0ZWQuCisgKiBUaGUgZnVuY3Rpb24K
KyAqIDEuIHZhbGlkYXRlcyBjb25uZWN0aW9uIGluZm8gc3RyaW5nIGFuZAorICogMi4gY2hlY2tz
IGEgcGFzc3dvcmQgaXMgcHJvdmlkZWQgaWYgbXVzdF91c2VfcGFzc3dvcmQgaXMgdHJ1ZS4KICAq
CiAgKiBJZiB0aGUgY29ubmVjdGlvbiBzdHJpbmcgY2FuJ3QgYmUgcGFyc2VkLCB0aGlzIGZ1bmN0
aW9uIHdpbGwgcmFpc2UKLSAqIGFuIGVycm9yIGFuZCB3aWxsIG5vdCByZXR1cm4uIElmIGl0IGNh
biwgaXQgd2lsbCByZXR1cm4gdHJ1ZSBpZiB0aGlzCi0gKiBjb25uZWN0aW9uIHN0cmluZyBzcGVj
aWZpZXMgYSBwYXNzd29yZCBhbmQgZmFsc2Ugb3RoZXJ3aXNlLgorICogYW4gZXJyb3IuIElmIG11
c3RfdXNlX3Bhc3N3b3JkIGlzIHRydWUsIHRoZSBmdW5jdGlvbiByYWlzZXMgYW4gZXJyb3IKKyAq
IGlmIG5vIHBhc3N3b3JkIGlzIHByb3ZpZGVkIGluIHRoZSBjb25uZWN0aW9uIHN0cmluZy4gSW4g
YW55IG90aGVyIGNhc2UKKyAqIGl0IHN1Y2Nlc3NmdWxseSBjb21wbGV0ZXMuCiAgKi8KIHN0YXRp
YyB2b2lkCiBsaWJwcXJjdl9jaGVja19jb25uaW5mbyhjb25zdCBjaGFyICpjb25uaW5mbywgYm9v
bCBtdXN0X3VzZV9wYXNzd29yZCkKLS0gCjIuMzkuMyAoQXBwbGUgR2l0LTE0NikKCg==
--=_20e8bc327f30acfcd0754f348d90586a--