Re: Fix md5_password_warnings for role/database settings

From: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
To: Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
Cc: Japin Li <japinli(at)hotmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Nathan Bossart <nathandbossart(at)gmail(dot)com>, Darold Gilles <gilles(at)darold(dot)net>
Subject: Re: Fix md5_password_warnings for role/database settings
Date: 2026-06-10 23:56:59
Message-ID: 60C3EDF4-7571-4749-9EE3-9D427FE563BB@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Jun 10, 2026, at 23:12, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> On Wed, Jun 10, 2026 at 8:58 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
>>> PSA v2 - addressed Japin’s first comment.
>>>
>> Thanks for updating the patch, LGTM.
>
> The patch basically looks good to me!

Thanks for your review.

> I just have three minor review comments.
>
>
> + my ($ret, $stdout, $stderr) = $node->psql(
> + 'postgres',
> + 'SELECT 1',
> + connstr => 'user=md5_role_no_warnings',
> + extra_params => ['-w'],
> + on_error_stop => 0);
> + is($ret, 0, 'md5 with warnings disabled');
> + unlike(
> + $stderr,
> + qr/authenticated with an MD5-encrypted password/,
> + 'md5 with warnings disabled: no MD5 authentication warning');
>
> For this test, would it be better to use connect_ok() instead of
> psql(), like this? That would let us verify more behavior at once:
> the connection succeeds, stderr is empty (i.e., no MD5 warning is
> emitted), SHOW md5_password_warnings returns off, and the server
> log still shows method=md5.
>
> $node->connect_ok(
> "user=md5_role_no_warnings",
> "md5 with warnings disabled",
> sql => "SHOW md5_password_warnings",
> expected_stdout => qr/^off$/,
> log_like =>
> [qr/connection authenticated: identity="md5_role_no_warnings" method=md5/]);
>

Agreed.

>
> * NB: Caller should ensure the strings are allocated in a long-lived context
> - * like TopMemoryContext.
> + * like TopMemoryContext. This function takes ownership of the strings, which
> + * will be freed in EmitConnectionWarnings().
>
> Very minor comment: I'd suggest changing "allocated" to "palloc'd"
> and "freed" to "pfree'd", to avoid future callers passing some non-pfreeable
> memory unexpectedly.
>

Okay, changed.

>
> +typedef struct ConnectionWarning
>
> Since this patch introduces a new typedef, ConnectionWarning,
> typedefs.list should probably be updated as well. Even if we forget
> to do that, it will eventually get updated later, but I think it's
> better to update it manually when possible so that pgindent works
> correctly before then.
>

Ah, completely forgot that. Added.

PFA v3:
* Addressed all Fujii’s comments
* Changed a palloc to palloc_object

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

Attachment Content-Type Size
v3-0001-Fix-md5_password_warnings-for-role-and-database-s.patch application/octet-stream 8.9 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Smith 2026-06-11 00:04:03 Re: Add pg_get_publication_ddl function
Previous Message Jacob Champion 2026-06-10 23:42:22 Re: Heads Up: cirrus-ci is shutting down June 1st