> On Feb 5, 2026, at 06:15, Nathan Bossart <[email protected]> wrote:
>
> On Wed, Feb 04, 2026 at 06:12:07PM -0300, Euler Taveira wrote:
>> That's correct. You should use ngettext(). Using the plural form means better
>> translation. Looking at some messages in the catalog, the developers tend to
>> ignore the fact that the sentence has a plural form too. Sometimes it is hard
>> to write a message if multiple parts of the message have plural form. I
>> completely understand your resistance.
>
> Please pardon the brain fade; I'd forgotten about ngettext() and missed
> your previous message. Here is an updated patch.
>
> --
> nathan
> <v16-0001-Add-password-expiration-warnings.patch>
Hi Nathan,
Thanks for the patch. I think this is a very helpful addition. I’ve reviewed
v16 and it looks solid overall. I only have a few small comments.
1
```
+{ name => 'password_expiration_warning_threshold', type => 'int', context =>
'PGC_SIGHUP', group => 'CONN_AUTH_AUTH',
+ short_desc => 'Time before password expiration warnings.',
+ long_desc => '0 means not to emit these warnings.',
+ flags => 'GUC_UNIT_S',
+ variable => 'password_expiration_warning_threshold',
+ boot_val => '604800',
+ min => '0',
+ max => 'INT_MAX',
+},
```
* The value `604800` is used in two places: here in the GUC definition and in
`crypt.c` to initialize `password_expiration_warning_threshold`. It might be
cleaner to define a shared constant (e.g., a macro in `crypt.h`) and reuse it
in both places.
* Using `INT_MAX` as the upper bound feels a bit meaningless. Would it make
sense to cap this at a more reasonable value (say 30, 60, or 180 days)? That
could help catch accidental misconfiguration — for example, typing `70d`
instead of `7d`.
2
```
+#password_expiration_warning_threshold = 7d # time before expiration warnings
```
In postgresql.conf.sample, should we also mention that setting this to 0
disables the warning?
3
```
+void
+StoreConnectionWarning(char *msg, char *detail)
+{
+ MemoryContext oldcontext;
+
+ Assert(msg);
+
+ if (ConnectionWarningsEmitted)
+ elog(ERROR, "StoreConnectionWarning() called after
EmitConnectionWarnings()");
+
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+ MyProcPort->warning_msgs = lappend(MyProcPort->warning_msgs, msg);
+ MyProcPort->warning_details = lappend(MyProcPort->warning_details,
detail);
+
+ MemoryContextSwitchTo(oldcontext);
+}
```
Switching to TopMemoryContext here makes sense to ensure the warnings survive
until the end of InitPostgres(). However, this still relies on callers to
allocate msg and detail in a sufficiently long-lived context, which isn’t
guaranteed.
I wonder if it would be better for this function to pstrdup() both msg and
detail internally, so that ownership and lifetime are fully contained here. If
so, the arguments could also be declared as `const char *`.
With that change, the explicit TopMemoryContext switch in get_role_password()
could likely be avoided.
4
```
+ /*
+ * If we're past rolvaliduntil, the connection attempt should
fail, so
+ * update logdetail and return NULL.
+ */
+ if (expire_time < 0)
+ {
+ *logdetail = psprintf(_("User \"%s\" has an expired
password."),
+ role);
+ return NULL;
+ }
```
Should this be `expire_time <= 0` instead? As written, when `expire_time == 0`
the user would get a warning like “password will expire in less than 1 minute”,
which feels slightly odd.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
