Re: Fix md5_password_warnings for role/database settings

> On Jun 10, 2026, at 18:22, Japin Li <japinli@hotmail.com> wrote:
> 
> 
> Hi, Chao
> 
> On Wed, 10 Jun 2026 at 14:26, Chao Li <li.evan.chao@gmail.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/