Re: Pasword expiration warning

On Thu, Feb 05, 2026 at 09:06:27AM +0800, Chao Li wrote:
> 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.

Thanks for reviewing.

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

Eh, there are lots of examples of setting the default value in both the GUC
definition and the variable declaration. TBH I've always found excessive
macro use to hinder readability more than it helps anything. Sure, if we
need to change the default, we'll have to update both places, but we
already have to update the docs and tests and sample file, anyway.

> * 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`.

On the contrary, it's quite meaningful. It means that there's no technical
restriction for arbitrarily large values and that users are free to set it
to whatever value makes sense for them.

> In postgresql.conf.sample, should we also mention that setting this to 0
> disables the warning?

I don't think it's necessary. IMHO it's reasonably obvious that setting
the value to 0 means there's effectively 0 time between enabling the
warnings and the password expiring.

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

StoreConnectionWarning() has the following comment:

* NB: Caller should ensure the strings are allocated in a long-lived context
* like TopMemoryContext.

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

Currently, we consider the password as expired when

vuntil < GetCurrentTimestamp()

... which matches the documented behavior:

The VALID UNTIL clause sets a date and time after which the role's
password is no longer valid.

I see no need to change it.

--
nathan