Re: Fix md5_password_warnings for role/database settings

> On Jun 10, 2026, at 18:22, Japin Li <japinli(at)hotmail(dot)com> wrote:
>
>
> Hi, Chao
>
> On Wed, 10 Jun 2026 at 14:26, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>> Hi,
>>
>> While testing “[bc60ee860] Warn upon successful MD5 password authentication”, I found a small issue.
>>
>> This feature emits a warning based on the existing GUC
>> md5_password_warnings, but it queues the message in
>> md5_crypt_verify(), before GUC values are loaded by
>> process_startup_options() and process_settings(). As a result,
>> settings loaded later during connection startup, such as startup
>> options or ALTER ROLE/ALTER DATABASE settings, are not honored for
>> this warning.
>>
>> Here is a repro:
>>
>> 1. Edit pg_hba.conf, add this line:
>> ```
>> local postgres md5_role md5
>> ```
>>
>> 2. Setup in session 1:
>> ```
>> evantest=# set password_encryption='md5';
>> SET
>> evantest=# create role md5_role login password 'pass';
>> WARNING: setting an MD5-encrypted password
>> DETAIL: MD5 password support is deprecated and will be removed in a future release of PostgreSQL.
>> HINT: Refer to the PostgreSQL documentation for details about migrating to another password type.
>> CREATE ROLE
>> evantest=#
>> evantest=# alter role md5_role set md5_password_warnings =0;
>> ALTER ROLE
>> evantest=# select pg_reload_conf(); -- reload pg_hba.conf as I didn’t restart the server
>> pg_reload_conf
>> ----------------
>> t
>> (1 row)
>> ```
>>
>> 3. Connect as md5_role:
>> ```
>> % PGPASSWORD=pass psql -d postgres -U md5_role -X -qAt -c “show md5_password_warnings"
>> WARNING: authenticated with an MD5-encrypted password
>> DETAIL: MD5 password support is deprecated and will be removed in a future release of PostgreSQL.
>> off
>> ```
>>
>> As we can see, although the role’s md5_password_warnings setting is off, the warning message is still shown.
>>
>> This feature uses the connection warning infrastructure introduced by 1d92e0c2cc, so fixing the problem requires enhancing that infrastructure.
>>
>> In the current implementation, there are two lists:
>> ConnectionWarningMessages and ConnectionWarningDetails. The attached
>> patch combines them into one list and adds a filter function to each
>> list member, so the filter can be applied in
>> EmitConnectionWarnings(). With this mechanism, the warning emitted
>> upon successful MD5 authentication is checked against the final value
>> of md5_password_warnings, while 1d92e0c2cc’s password expiration
>> warning logic remains unchanged.
>>
>
> I'm in favor of this idea.

Thanks for your review.

>
>> See the attached patch for details.
>
> A few comments:
>
> 1.
> EmitConnectionWarnings(void)
> {
> - ListCell *lc_msg;
> - ListCell *lc_detail;
> + ListCell *lc;
>
> if (ConnectionWarningsEmitted)
> elog(ERROR, "EmitConnectionWarnings() called more than once");
> else
> ConnectionWarningsEmitted = true;
>
> - forboth(lc_msg, ConnectionWarningMessages,
> - lc_detail, ConnectionWarningDetails)
> + foreach(lc, ConnectionWarnings)
> {
> - ereport(WARNING,
> - (errmsg("%s", (char *) lfirst(lc_msg)),
> - errdetail("%s", (char *) lfirst(lc_detail))));
> + ConnectionWarning *warning = lfirst(lc);
> +
>
> Perhaps we could use foreach_ptr(ConnectionWarning, warning, ConnectionWarnings)
> to simplify the code.

Agreed.

>
> 2.
> StoreConnectionWarning() states that the caller should ensure the strings are
> allocated in a long-lived context. Since the two existing calls already use
> TopMemoryContext, should the function always switch the memory context
> internally?
>

I raised the same comment when I reviewed the original patch of 1d92e0c2cc, and the comment was addressed by adding the header comment, see [1] commit 3. So, I’d not touch this part.

PSA v2 - addressed Japin’s first comment.

[1] https://postgr.es/m/6E83A384-89B0-4141-887F-E54C02B2CACE@gmail.com

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/