On Wed, Jun 10, 2026 at 8:58 PM Japin Li <[email protected]> wrote: > > PSA v2 - addressed Japin’s first comment. > > > Thanks for updating the patch, LGTM.
The patch basically looks good to me! 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/]); * 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. +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. Regards, -- Fujii Masao
