| 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 |
| 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 |