Comment for UserMappingPasswordRequired in contrib/postgres_fdw
Hi,
While working on something else, I noticed $SUBJECT:
/*
* Return true if the password_required is defined and false for this user
* mapping, otherwise false. The mapping has been pre-validated.
*/
static bool
UserMappingPasswordRequired(UserMapping *user)
{
ListCell *cell;
foreach(cell, user->options)
{
DefElem *def = (DefElem *) lfirst(cell);
if (strcmp(def->defname, "password_required") == 0)
return defGetBoolean(def);
}
return true;
}
I think the former part of the comment should be: Return *false* if
the password_required is defined and false for this user mapping,
otherwise *true*.
Patch attached.
Best regards,
Etsuro Fujita
Attachments:
comment-fix.patchapplication/octet-stream; name=comment-fix.patchDownload+2-2
On 2/18/26 9:23 PM, Etsuro Fujita wrote:
I think the former part of the comment should be: Return *false* if
the password_required is defined and false for this user mapping,
otherwise *true*.
I feel the wording of the comment is pretty awkward both before and
after your correctness fix. I am not a native speaker but shouldn't it
be something like the below which explains better what is actually going on.
/*
* Checks the value of password_required, defaults to true
* if not defined. The mapping has been pre-validated.
*/
Andreas
On Thu, Feb 19, 2026 at 5:30 AM Andreas Karlsson <andreas@proxel.se> wrote:
On 2/18/26 9:23 PM, Etsuro Fujita wrote:
I think the former part of the comment should be: Return *false* if
the password_required is defined and false for this user mapping,
otherwise *true*.I feel the wording of the comment is pretty awkward both before and
after your correctness fix. I am not a native speaker but shouldn't it
be something like the below which explains better what is actually going on./*
* Checks the value of password_required, defaults to true
* if not defined. The mapping has been pre-validated.
*/
I like your wording. I am not a native speaker either, though. This
would be nitpicking, but I think it is better to clearly mention what
the function returns. How about modifying it a bit, like this?
/*
* Check and return the value of password_required, if defined; otherwise,
* return true, which is the default value of it. The mapping has been
* pre-validated.
*/
Anyway, thanks for the comment!
Best regards,
Etsuro Fujita
On 2/22/26 12:10 PM, Etsuro Fujita wrote:
/*
* Checks the value of password_required, defaults to true
* if not defined. The mapping has been pre-validated.
*/I like your wording. I am not a native speaker either, though. This
would be nitpicking, but I think it is better to clearly mention what
the function returns. How about modifying it a bit, like this?/*
* Check and return the value of password_required, if defined; otherwise,
* return true, which is the default value of it. The mapping has been
* pre-validated.
*/
No strong opinion. I would be fine with either. I do not think saying
that it returns is necessary since you can see that from the function
definition but it does not really harm either.
--
Andreas Karlsson
Percona
On Mon, Feb 23, 2026 at 8:01 AM Andreas Karlsson <andreas@proxel.se> wrote:
On 2/22/26 12:10 PM, Etsuro Fujita wrote:
/*
* Checks the value of password_required, defaults to true
* if not defined. The mapping has been pre-validated.
*/I like your wording. I am not a native speaker either, though. This
would be nitpicking, but I think it is better to clearly mention what
the function returns. How about modifying it a bit, like this?/*
* Check and return the value of password_required, if defined; otherwise,
* return true, which is the default value of it. The mapping has been
* pre-validated.
*/No strong opinion. I would be fine with either. I do not think saying
that it returns is necessary since you can see that from the function
definition but it does not really harm either.
Ok. I like the modified version as it also keeps the existing comment
to some extent, so I'd like to go with it. Updated patch attached. I
will push and back-patch it to all supported versions if there are no
objections from others.
Thanks!
Best regards,
Etsuro Fujita
Attachments:
comment-fix-2.patchapplication/octet-stream; name=comment-fix-2.patchDownload+3-2
On Mon, Feb 23, 2026 at 6:36 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
On Mon, Feb 23, 2026 at 8:01 AM Andreas Karlsson <andreas@proxel.se> wrote:
On 2/22/26 12:10 PM, Etsuro Fujita wrote:
/*
* Checks the value of password_required, defaults to true
* if not defined. The mapping has been pre-validated.
*/I like your wording. I am not a native speaker either, though. This
would be nitpicking, but I think it is better to clearly mention what
the function returns. How about modifying it a bit, like this?/*
* Check and return the value of password_required, if defined; otherwise,
* return true, which is the default value of it. The mapping has been
* pre-validated.
*/No strong opinion. I would be fine with either. I do not think saying
that it returns is necessary since you can see that from the function
definition but it does not really harm either.Ok. I like the modified version as it also keeps the existing comment
to some extent, so I'd like to go with it. Updated patch attached. I
will push and back-patch it to all supported versions if there are no
objections from others.
Done.
Best regards,
Etsuro Fujita