Thread
Commits
-
Add password expiration warnings.
- 1d92e0c2cc47 19 (unreleased) landed
-
Pasword expiration warning
Gilles Darold <gilles@migops.com> — 2021-11-19T14:49:37Z
Hi all, Now that the security policy is getting stronger, it is not uncommon to create users with a password expiration date (VALID UNTIL). The problem is that the user is only aware that his password has expired when he can no longer log in unless the application with which he is connecting notifies him beforehand. I'm wondering if we might be interested in having this feature in psql? For example for a user whose password expires in 3 days: gilles=# CREATE ROLE foo LOGIN PASSWORD 'foo' VALID UNTIL '2021-11-22'; CREATE ROLE gilles=# \c - foo Password for user foo: psql (15devel, server 14.1 (Ubuntu 14.1-2.pgdg20.04+1)) ** Warning: your password expires in 3 days ** You are now connected to database "gilles" as user "foo". My idea is to add a psql variable that can be defined in psqlrc to specify the number of days before the user password expires to start printing a warning. The warning message is only diplayed in interactive mode Example: $ cat /etc/postgresql-common/psqlrc \set PASSWORD_EXPIRE_WARNING 7 Default value is 0 like today no warning at all. Of course any other client application have to write his own beforehand expiration notice but with psql we don't have it for the moment. If there is interest for this psql feature I can post the patch. -- Gilles Darold
-
Re: Pasword expiration warning
Dinesh Chemuduru <dinesh.kumar@migops.com> — 2021-11-19T15:16:29Z
On Fri, 19 Nov 2021 at 20:19, Gilles Darold <gilles@migops.com> wrote: > Hi all, > > > Now that the security policy is getting stronger, it is not uncommon to > create users with a password expiration date (VALID UNTIL). The problem > is that the user is only aware that his password has expired when he can no > longer log in unless the application with which he is connecting notifies > him beforehand. > > > I'm wondering if we might be interested in having this feature in psql? For > example for a user whose password expires in 3 days: > > gilles=# CREATE ROLE foo LOGIN PASSWORD 'foo' VALID UNTIL '2021-11-22'; > CREATE ROLE > gilles=# \c - foo > Password for user foo: > psql (15devel, server 14.1 (Ubuntu 14.1-2.pgdg20.04+1)) > ** Warning: your password expires in 3 days ** > You are now connected to database "gilles" as user "foo". > > > My idea is to add a psql variable that can be defined in psqlrc to specify > the number of days before the user password expires to start printing a > warning. The warning message is only diplayed in interactive mode Example: > > $ cat /etc/postgresql-common/psqlrc > \set PASSWORD_EXPIRE_WARNING 7 > > +1 It is useful to notify the users about their near account expiration, and we are doing that at client level. Default value is 0 like today no warning at all. > > > Of course any other client application have to write his own beforehand expiration > notice but with psql we don't have it for the moment. If there is interest > for this psql feature I can post the patch. > > -- > Gilles Darold > >
-
Re: Pasword expiration warning
Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-19T15:55:34Z
Gilles Darold <gilles@migops.com> writes: > Now that the security policy is getting stronger, it is not uncommon to > create users with a password expiration date (VALID UNTIL). TBH, I thought people were starting to realize that forced password rotations are a net security negative. It's true that a lot of places haven't gotten the word yet. > I'm wondering if we might be interested in having this feature in psql? This proposal kind of seems like a hack, because (1) not everybody uses psql (2) psql can't really tell whether rolvaliduntil is relevant. (It can see whether the server demanded a password, but maybe that went to LDAP or some other auth method.) That leads me to wonder about server-side solutions. It's easy enough for the server to see that it's used a password with an expiration N days away, but how could that be reported to the client? The only idea that comes to mind that doesn't seem like a protocol break is to issue a NOTICE message, which doesn't seem like it squares with your desire to only do this interactively. (Although I'm not sure I believe that's a great idea. If your application breaks at 2AM because its password expired, you won't be any happier than if your interactive sessions start to fail. Maybe a message that would leave a trail in the server log would be best after all.) > Default value is 0 like today no warning at all. Off-by-default is pretty much guaranteed to not help most people. regards, tom lane -
Re: Pasword expiration warning
Gilles Darold <gilles@migops.com> — 2021-11-19T16:56:20Z
Le 19/11/2021 à 16:55, Tom Lane a écrit : > Gilles Darold <gilles@migops.com> writes: >> Now that the security policy is getting stronger, it is not uncommon to >> create users with a password expiration date (VALID UNTIL). > TBH, I thought people were starting to realize that forced password > rotations are a net security negative. It's true that a lot of > places haven't gotten the word yet. > >> I'm wondering if we might be interested in having this feature in psql? > This proposal kind of seems like a hack, because > (1) not everybody uses psql Yes, for me it's a comfort feature. When a user connect to a PG backend using an account that have expired you have no information that the problem is a password expiration. The message returned to the user is just: "FATAL: password authentication failed for user "foo". We had to verify in the log file that the problem is related to "DETAIL: User "foo" has an expired password.". If the user was warned beforehand to change the password it will probably saves me some time. > (2) psql can't really tell whether rolvaliduntil is relevant. > (It can see whether the server demanded a password, but > maybe that went to LDAP or some other auth method.) I agree, I hope that in case of external authentication rolvaliduntil is not set and in this case I guess that there is other notification channels to inform the user that his password will expire. Otherwise yes the warning message could be a false positive but the rolvaliduntil can be changed to infinity to fix this case. > That leads me to wonder about server-side solutions. It's easy > enough for the server to see that it's used a password with an > expiration N days away, but how could that be reported to the > client? The only idea that comes to mind that doesn't seem like > a protocol break is to issue a NOTICE message, which doesn't > seem like it squares with your desire to only do this interactively. > (Although I'm not sure I believe that's a great idea. If your > application breaks at 2AM because its password expired, you > won't be any happier than if your interactive sessions start to > fail. Maybe a message that would leave a trail in the server log > would be best after all.) I think that this is the responsibility of the client to display a warning when the password is about to expire, the backend could help the application by sending a NOTICE but the application will still have to report the notice. I mean that it can continue to do all the work to verify that the password is about to expire. >> Default value is 0 like today no warning at all. > Off-by-default is pretty much guaranteed to not help most people. Right, I was thinking of backward compatibility but this does not apply here. So default to 7 days will be better. To sum up as I said on top this is just a comfort notification dedicated to psql and for local pg account to avoid looking at log file for forgetting users. -- Gilles Darold
-
Re: Pasword expiration warning
Nathan Bossart <bossartn@amazon.com> — 2021-11-20T00:17:53Z
On 11/19/21, 7:56 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: > That leads me to wonder about server-side solutions. It's easy > enough for the server to see that it's used a password with an > expiration N days away, but how could that be reported to the > client? The only idea that comes to mind that doesn't seem like > a protocol break is to issue a NOTICE message, which doesn't > seem like it squares with your desire to only do this interactively. > (Although I'm not sure I believe that's a great idea. If your > application breaks at 2AM because its password expired, you > won't be any happier than if your interactive sessions start to > fail. Maybe a message that would leave a trail in the server log > would be best after all.) I bet it's possible to use the ClientAuthentication_hook for this. In any case, I agree that it probably belongs server-side so that other clients can benefit from this. Nathan
-
Re: Pasword expiration warning
Michael Paquier <michael@paquier.xyz> — 2021-11-20T05:46:02Z
On Sat, Nov 20, 2021 at 12:17:53AM +0000, Bossart, Nathan wrote: > I bet it's possible to use the ClientAuthentication_hook for this. In > any case, I agree that it probably belongs server-side so that other > clients can benefit from this. ClientAuthentication_hook is called before the user is informed of the authentication result, FWIW, so that does not seem wise. -- Michael
-
Re: Pasword expiration warning
Andrew Dunstan <andrew@dunslane.net> — 2021-11-20T13:48:35Z
On 11/19/21 19:17, Bossart, Nathan wrote: > On 11/19/21, 7:56 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: >> That leads me to wonder about server-side solutions. It's easy >> enough for the server to see that it's used a password with an >> expiration N days away, but how could that be reported to the >> client? The only idea that comes to mind that doesn't seem like >> a protocol break is to issue a NOTICE message, which doesn't >> seem like it squares with your desire to only do this interactively. >> (Although I'm not sure I believe that's a great idea. If your >> application breaks at 2AM because its password expired, you >> won't be any happier than if your interactive sessions start to >> fail. Maybe a message that would leave a trail in the server log >> would be best after all.) > I bet it's possible to use the ClientAuthentication_hook for this. In > any case, I agree that it probably belongs server-side so that other > clients can benefit from this. > +1 for a server side solution. The people most likely to benefit from this are the people least likely to be using psql IMNSHO. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
-
Re: Pasword expiration warning
Gilles Darold <gilles@migops.com> — 2021-11-21T09:49:57Z
Le 20/11/2021 à 14:48, Andrew Dunstan a écrit : > On 11/19/21 19:17, Bossart, Nathan wrote: >> On 11/19/21, 7:56 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: >>> That leads me to wonder about server-side solutions. It's easy >>> enough for the server to see that it's used a password with an >>> expiration N days away, but how could that be reported to the >>> client? The only idea that comes to mind that doesn't seem like >>> a protocol break is to issue a NOTICE message, which doesn't >>> seem like it squares with your desire to only do this interactively. >>> (Although I'm not sure I believe that's a great idea. If your >>> application breaks at 2AM because its password expired, you >>> won't be any happier than if your interactive sessions start to >>> fail. Maybe a message that would leave a trail in the server log >>> would be best after all.) >> I bet it's possible to use the ClientAuthentication_hook for this. In >> any case, I agree that it probably belongs server-side so that other >> clients can benefit from this. >> > +1 for a server side solution. The people most likely to benefit from > this are the people least likely to be using psql IMNSHO. Ok, I can try to implement something at server side using a NOTICE message. -- Gilles Darold
-
Re: Pasword expiration warning
Gilles DAROLD <gilles@darold.net> — 2026-01-06T14:43:20Z
Le 21/11/2021 à 10:49, Gilles Darold a écrit : > Le 20/11/2021 à 14:48, Andrew Dunstan a écrit : >> On 11/19/21 19:17, Bossart, Nathan wrote: >>> On 11/19/21, 7:56 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: >>>> That leads me to wonder about server-side solutions. It's easy >>>> enough for the server to see that it's used a password with an >>>> expiration N days away, but how could that be reported to the >>>> client? The only idea that comes to mind that doesn't seem like >>>> a protocol break is to issue a NOTICE message, which doesn't >>>> seem like it squares with your desire to only do this interactively. >>>> (Although I'm not sure I believe that's a great idea. If your >>>> application breaks at 2AM because its password expired, you >>>> won't be any happier than if your interactive sessions start to >>>> fail. Maybe a message that would leave a trail in the server log >>>> would be best after all.) >>> I bet it's possible to use the ClientAuthentication_hook for this. In >>> any case, I agree that it probably belongs server-side so that other >>> clients can benefit from this. >>> >> +1 for a server side solution. The people most likely to benefit from >> this are the people least likely to be using psql IMNSHO. >> >> >> Ok, I can try to implement something at server side using a NOTICE message. Hi, Sorry to resurrect this old thread, but I had completely forgotten about it. If there's still interest in this feature, then please find in attachment a patch to emit a warning to the client and into the logs when the password will expire within 7 days by default. A GUC, password_expire_warning, allow to change the number of days before sending the message or to disable this feature with setting value 0. I have chosen to add a new field, const char *warning_message, to struct ClientConnectionInfo so that it can be used to send other messages to the client at end of connection ( src/backend/utils/init/postinit.c: InitPostgres() ). Not sure sure that this is the best way to do that but as it is a message dedicated to the connection I've though it could be the right place. If we don't expect other warning message sent to the client at connection time, just using an integer for the number of days remaining will be enough. We could use notice but it is not logged by default and also I think that warning is the good level for this message. Output at psql connection: $ /usr/local/pgsql/bin/psql -h localhost -U test -d postgres Password for user test: WARNING: your password will expire in 4 days psql (19devel) Type "help" for help. postgres=> Output in the log: 2026-01-05 23:23:13.763 CET [136001] WARNING: your password will expire in 4 days Using a script: $ perl test_conn.pl WARNING: your password will expire in 3 days The message can be handled by any client application to warn the user if required. Thanks in advance for your feedback and suggestion for a better implementation. Best regards, -- Gilles Darold http://hexacluster.ai/
-
Re: Pasword expiration warning
Japin Li <japinli@hotmail.com> — 2026-01-07T05:12:57Z
Hi, Gilles Darold On Tue, 06 Jan 2026 at 15:43, Gilles Darold <gilles@darold.net> wrote: > Le 21/11/2021 à 10:49, Gilles Darold a écrit : >> Le 20/11/2021 à 14:48, Andrew Dunstan a écrit : >>> On 11/19/21 19:17, Bossart, Nathan wrote: >>>> On 11/19/21, 7:56 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: >>>>> That leads me to wonder about server-side solutions. It's easy >>>>> enough for the server to see that it's used a password with an >>>>> expiration N days away, but how could that be reported to the >>>>> client? The only idea that comes to mind that doesn't seem like >>>>> a protocol break is to issue a NOTICE message, which doesn't >>>>> seem like it squares with your desire to only do this interactively. >>>>> (Although I'm not sure I believe that's a great idea. If your >>>>> application breaks at 2AM because its password expired, you >>>>> won't be any happier than if your interactive sessions start to >>>>> fail. Maybe a message that would leave a trail in the server log >>>>> would be best after all.) >>>> I bet it's possible to use the ClientAuthentication_hook for this. In >>>> any case, I agree that it probably belongs server-side so that other >>>> clients can benefit from this. >>>> >>> +1 for a server side solution. The people most likely to benefit from >>> this are the people least likely to be using psql IMNSHO. >>> >>> >>> Ok, I can try to implement something at server side using a NOTICE message. > > Hi, > > Sorry to resurrect this old thread, but I had completely forgotten > about it. If there's still interest in this feature, then please find > in attachment a patch to emit a warning to the client and into the > logs when the password will expire within 7 days by default. A GUC, > password_expire_warning, allow to change the number of days before > sending the message or to disable this feature with setting value 0. > > I have chosen to add a new field, const char *warning_message, to > struct ClientConnectionInfo so that it can be used to send other > messages to the client at end of connection ( > src/backend/utils/init/postinit.c: InitPostgres() ). Not sure sure > that this is the best way to do that but as it is a message dedicated > to the connection I've though it could be the right place. If we don't > expect other warning message sent to the client at connection time, > just using an integer for the number of days remaining will be > enough. We could use notice but it is not logged by default and also I > think that warning is the good level for this message. > > Output at psql connection: > > $ /usr/local/pgsql/bin/psql -h localhost -U test -d postgres > Password for user test: > WARNING: your password will expire in 4 days > psql (19devel) > Type "help" for help. > > postgres=> > > Output in the log: > > 2026-01-05 23:23:13.763 CET [136001] WARNING: your password > will expire in 4 days > > Using a script: > > $ perl test_conn.pl > WARNING: your password will expire in 3 days > > The message can be handled by any client application to warn the user > if required. > > > Thanks in advance for your feedback and suggestion for a better > implementation. > Thanks for updating the patch. 1. $ git apply ~/password_expire_warning-v1.patch /home/japin/password_expire_warning-v1.patch:71: indent with spaces. if (MyClientConnectionInfo.warning_message) /home/japin/password_expire_warning-v1.patch:72: indent with spaces. ereport(WARNING, (errmsg("%s", MyClientConnectionInfo.warning_message))); warning: 2 lines add whitespace errors. 2. +{ name => 'password_expire_warning', type => 'int', context => 'PGC_SIGHUP', group => 'CONN_AUTH_AUTH', + short_desc => 'Sets the number of days before password expire to emit a warning at client connection. Default is 7 days, 0 means no warning.', + flags => 'GUC_UNIT_S', + variable => 'password_expire_warning', + boot_val => '7', + min => '0', + max => '30', +}, + The GUC_UNIT_S flag specifies that the unit is seconds, meaning the default value of 7 corresponds to 7 seconds rather than 7 days. For example: # ALTER SYSTEM SET password_expire_warning TO 60; ERROR: 60 s is outside the valid range for parameter "password_expire_warning" (0 s .. 30 s) 3. I tested the patch and only received an empty WARNING message. After some analysis, I found that the warning_message buffer is likely freed after transaction commit. Here's a new patch that resolves the issues mentioned earlier. Furthermore, if the remaining time until expiration is less than one day, should we display it in minutes (or hours) instead of 0 days? -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd. -
Re: Pasword expiration warning
songjinzhou <tsinghualucky912@foxmail.com> — 2026-01-07T08:37:23Z
Hello fellow hackers, I've refined the print information based on the japin code above, but otherwise remained unchanged. The result is as follows: [postgres@localhost:~/test/bin]$ ./psql -p 5432 -U test_user2 -d postgres Password for user test_user2: 2026-01-07 00:28:25.999 PST [82198] WARNING: your password will expire in 7 hours and 31 minutes WARNING: your password will expire in 7 hours and 31 minutes psql (19devel) Type "help" for help. postgres=> \q [postgres@localhost:~/test/bin]$ ./psql -p 5432 -U test_user3 -d postgres Password for user test_user3: 2026-01-07 00:28:33.998 PST [82282] WARNING: your password will expire in 2 days and 7 hours WARNING: your password will expire in 2 days and 7 hours psql (19devel) Type "help" for help. postgres=> \q [postgres@localhost:~/test/bin]$ Thanks songjinzhou tsinghualucky912@foxmail.com
-
Re: Pasword expiration warning
Gilles DAROLD <gilles@darold.net> — 2026-01-07T14:49:02Z
Le 07/01/2026 à 06:12, Japin Li a écrit : > Thanks for updating the patch. > > 1. > $ git apply ~/password_expire_warning-v1.patch > /home/japin/password_expire_warning-v1.patch:71: indent with spaces. > if (MyClientConnectionInfo.warning_message) > /home/japin/password_expire_warning-v1.patch:72: indent with spaces. > ereport(WARNING, (errmsg("%s", MyClientConnectionInfo.warning_message))); > warning: 2 lines add whitespace errors. > > 2. > +{ name => 'password_expire_warning', type => 'int', context => 'PGC_SIGHUP', group => 'CONN_AUTH_AUTH', > + short_desc => 'Sets the number of days before password expire to emit a warning at client connection. Default is 7 days, 0 means no warning.', > + flags => 'GUC_UNIT_S', > + variable => 'password_expire_warning', > + boot_val => '7', > + min => '0', > + max => '30', > +}, > + > > The GUC_UNIT_S flag specifies that the unit is seconds, meaning the default > value of 7 corresponds to 7 seconds rather than 7 days. For example: > > # ALTER SYSTEM SET password_expire_warning TO 60; > ERROR: 60 s is outside the valid range for parameter "password_expire_warning" (0 s .. 30 s) > > 3. > I tested the patch and only received an empty WARNING message. After some > analysis, I found that the warning_message buffer is likely freed after > transaction commit. > > Here's a new patch that resolves the issues mentioned earlier. > > Furthermore, if the remaining time until expiration is less than one day, > should we display it in minutes (or hours) instead of 0 days? Thanks for the patch review and improvement. I missed the GUC_UNIT_S c/p but we can use second as unit. I have fixed your patch update because in this case the result variable must not be turned into days but kept in seconds to be compared to the GUC value. I have also added the missing GUC in the sample configuration file. I'm not in favor of a high granularity in time display, number of days for me is enough. I the user have the chance to see the 0 day remaining he knows that he must fix that immediately. But why not, it depends of a consensus. Thanks. -- Gilles Darold http://hexaculter.ai/ -
Re: Pasword expiration warning
Gilles Darold <gillesdarold@gmail.com> — 2026-01-07T14:56:32Z
Le 07/01/2026 à 09:37, songjinzhou a écrit : > Hello fellow hackers, I've refined the print information based on the japin code above, but otherwise remained unchanged. The result is as follows: > > [postgres@localhost:~/test/bin]$ ./psql -p 5432 -U test_user2 -d postgres > Password for user test_user2: > 2026-01-07 00:28:25.999 PST [82198] WARNING: your password will expire in 7 hours and 31 minutes > WARNING: your password will expire in 7 hours and 31 minutes > psql (19devel) > Type "help" for help. > > > postgres=> \q > [postgres@localhost:~/test/bin]$ ./psql -p 5432 -U test_user3 -d postgres > Password for user test_user3: > 2026-01-07 00:28:33.998 PST [82282] WARNING: your password will expire in 2 days and 7 hours > WARNING: your password will expire in 2 days and 7 hours > psql (19devel) > Type "help" for help. > > > postgres=> \q > [postgres@localhost:~/test/bin]$ > > > Thanks > > songjinzhou > tsinghualucky912@foxmail.com > I'm not in favor of a higher granularity like explained in my previous answer, I don't see it as a countdown to the second. But why not if there's more people in favor of a more detailed remaining time, your improvement will be welcome.
-
Re: Pasword expiration warning
songjinzhou <tsinghualucky912@foxmail.com> — 2026-01-08T02:57:00Z
Hi, Gilles Darold First of all, thank you for your reply. This is indeed not a simple countdown. I did think it would be abrupt for users to see "0 days". I checked v4, and I think it's fine. (PS: Should we add relevant explanations to the SGML?) Thank you. songjinzhou tsinghualucky912@foxmail.com
-
Re: Pasword expiration warning
Japin Li <japinli@hotmail.com> — 2026-01-08T03:37:42Z
On Thu, 08 Jan 2026 at 10:57, "songjinzhou" <tsinghualucky912@foxmail.com> wrote: > Hi, Gilles Darold > > First of all, thank you for your reply. This is indeed not a simple > countdown. I did think it would be abrupt for users to see "0 days". I > checked v4, and I think it's fine. (PS: Should we add relevant > explanations to the SGML?) Thank you. > I'd like to hear more opinions on this. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
-
Re: Pasword expiration warning
Gilles DAROLD <gilles@darold.net> — 2026-01-08T06:04:16Z
Le 08/01/2026 à 04:37, Japin Li a écrit : > On Thu, 08 Jan 2026 at 10:57, "songjinzhou" <tsinghualucky912@foxmail.com> wrote: >> Hi, Gilles Darold >> >> First of all, thank you for your reply. This is indeed not a simple >> countdown. I did think it would be abrupt for users to see "0 days". I >> checked v4, and I think it's fine. (PS: Should we add relevant >> explanations to the SGML?) Thank you. >> > I'd like to hear more opinions on this. > Here is a new version of the patch that adds the documentation for the new GUC, fix the warning message (days(s) instead of days) and handle the 'Infinity' value for the VALID UNTIL clause. -- Gilles Darold http://hexacluster.ai/
-
回复: Pasword expiration warning
liu xiaohui <liuxh.zj.cn@gmail.com> — 2026-01-08T07:33:01Z
________________________________ 发件人: Gilles Darold <gilles@darold.net> 发送时间: 2026年1月8日 14:04 收件人: Japin Li <japinli@hotmail.com>; songjinzhou <tsinghualucky912@foxmail.com> 抄送: Gilles Darold <gilles@darold.net>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>; Andrew Dunstan <andrew@dunslane.net>; Bossart, Nathan <bossartn@amazon.com>; Tom Lane <tgl@sss.pgh.pa.us> 主题: Re: Pasword expiration warning Le 08/01/2026 à 04:37, Japin Li a écrit : > On Thu, 08 Jan 2026 at 10:57, "songjinzhou" <tsinghualucky912@foxmail.com> wrote: >> Hi, Gilles Darold >> >> First of all, thank you for your reply. This is indeed not a simple >> countdown. I did think it would be abrupt for users to see "0 days". I >> checked v4, and I think it's fine. (PS: Should we add relevant >> explanations to the SGML?) Thank you. >> > I'd like to hear more opinions on this. > Here is a new version of the patch that adds the documentation for the new GUC, fix the warning message (days(s) instead of days) and handle the 'Infinity' value for the VALID UNTIL clause. -- Gilles Darold http://hexacluster.ai/ Dear Gilles, Thank you for submitting the v5 patch to add the password_expire_warning GUC. The feature is useful and the implementation is mostly solid. I reviewed the patch with a particular focus on the comments and documentation, and I noticed several inconsistencies between the comments, the documentation, and the actual code that could confuse future maintainers or users. Here are the main issues I found: 1. /doc/src/sgml/config.sgml There is an unrelated change in config.sgml around the password_encryption parameter: the closing parenthesis of the <type> tag was split onto its own line, resulting in an isolated ")". This appears to be an accidental editing artifact and is not required for the new feature. It should be reverted to keep the documentation formatting consistent with the rest of the file. 2. Comments referring to "days" while the internal variable uses seconds: src/backend/libpq/crypt.c: /* Emit a warning 7 days before password expiration */ These hard-code "7 days" may be no longer accurate once the value becomes configurable. Better comments would be: /* Threshold (in seconds) before password expiration to emit a warning at login (0 = disabled; default 7 days) */ 3. Minor typo/grammar in src/include/libpq/libpq-be.h In the comment for warning_message: "... at enf of InitPostgres(). ... it is use to show the password expiration warning." Should be: "at the end of InitPostgres()" and "it is used to show". Overall the implementation works correctly, but aligning all comments and documentation with the actual units (seconds internally, days for users) would greatly improve clarity. Best regards, Xiaohui Liu
-
Re: Pasword expiration warning
Japin Li <japinli@hotmail.com> — 2026-01-08T07:43:01Z
On Thu, 08 Jan 2026 at 07:04, Gilles Darold <gilles@darold.net> wrote: > Le 08/01/2026 à 04:37, Japin Li a écrit : >> On Thu, 08 Jan 2026 at 10:57, "songjinzhou" <tsinghualucky912@foxmail.com> wrote: >>> Hi, Gilles Darold >>> >>> First of all, thank you for your reply. This is indeed not a simple >>> countdown. I did think it would be abrupt for users to see "0 days". I >>> checked v4, and I think it's fine. (PS: Should we add relevant >>> explanations to the SGML?) Thank you. >>> >> I'd like to hear more opinions on this. >> > Here is a new version of the patch that adds the documentation for the > new GUC, fix the warning message (days(s) instead of days) and handle > the 'Infinity' value for the VALID UNTIL clause. > > Thanks for updating the patch. 1. I noticed a warning when applying the patch. Applying: Add password_expire_warning GUC to warn clients .git/rebase-apply/patch:31: trailing whitespace. disable this behavior. The default value is <literal>7d</literal>. warning: 1 line adds whitespace errors. 2. <varlistentry id="guc-password-encryption" xreflabel="password_encryption"> - <term><varname>password_encryption</varname> (<type>enum</type>) + <term><varname>password_encryption</varname> (<type>enum</type> +) I think this modification isn't necessary. 3. + float8 result; + + result = ((float8) (vuntil - GetCurrentTimestamp())) / 1000000.0; /* in seconds */ + Perhaps we could use TimestampTz for the result variable and substitute USECS_PER_SEC for 1000000.0—that would avoid the unnecessary type cast. 4. + if ((int) result <= password_expire_warning) If the result exceeds INT_MAX, it triggers undefined behavior (IIRC). Therefore, we should probably cast password_expire_warning itself. 5. With this feature, GetCurrentTimestamp() might end up being called twice. Perhaps we can avoid that duplication. Attached is v6 of the patch addressing the issues above. Please take a look. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd. -
Re: Pasword expiration warning
Gilles DAROLD <gilles@darold.net> — 2026-01-08T08:41:26Z
Le 08/01/2026 à 08:43, Japin Li a écrit : > On Thu, 08 Jan 2026 at 07:04, Gilles Darold <gilles@darold.net> wrote: >> Le 08/01/2026 à 04:37, Japin Li a écrit : >>> On Thu, 08 Jan 2026 at 10:57, "songjinzhou" <tsinghualucky912@foxmail.com> wrote: >>>> Hi, Gilles Darold >>>> >>>> First of all, thank you for your reply. This is indeed not a simple >>>> countdown. I did think it would be abrupt for users to see "0 days". I >>>> checked v4, and I think it's fine. (PS: Should we add relevant >>>> explanations to the SGML?) Thank you. >>>> >>> I'd like to hear more opinions on this. >>> >> Here is a new version of the patch that adds the documentation for the >> new GUC, fix the warning message (days(s) instead of days) and handle >> the 'Infinity' value for the VALID UNTIL clause. >> >> > Thanks for updating the patch. > > 1. > I noticed a warning when applying the patch. > > Applying: Add password_expire_warning GUC to warn clients > .git/rebase-apply/patch:31: trailing whitespace. > disable this behavior. The default value is <literal>7d</literal>. > warning: 1 line adds whitespace errors. > > 2. > <varlistentry id="guc-password-encryption" xreflabel="password_encryption"> > - <term><varname>password_encryption</varname> (<type>enum</type>) > + <term><varname>password_encryption</varname> (<type>enum</type> > +) > > I think this modification isn't necessary. > > 3. > + float8 result; > + > + result = ((float8) (vuntil - GetCurrentTimestamp())) / 1000000.0; /* in seconds */ > + > > Perhaps we could use TimestampTz for the result variable and substitute > USECS_PER_SEC for 1000000.0—that would avoid the unnecessary type cast. > > 4. > + if ((int) result <= password_expire_warning) > > If the result exceeds INT_MAX, it triggers undefined behavior (IIRC). > Therefore, we should probably cast password_expire_warning itself. > > 5. > With this feature, GetCurrentTimestamp() might end up being called twice. > Perhaps we can avoid that duplication. > > > Attached is v6 of the patch addressing the issues above. Please take a look. Thanks Japin, the implementation is fully working using the TimestampTz cast. I've attached a new patch to fix documentation and comments reported by liu xiaohui and create a commitfest entry : https://commitfest.postgresql.org/patch/6381/ Every one involved in the review should edit the commitfest entry. -- Gilles Darold http://hexacluster.ai/
-
Re: Pasword expiration warning
Japin Li <japinli@hotmail.com> — 2026-01-08T09:27:39Z
On Thu, 08 Jan 2026 at 09:41, Gilles Darold <gilles@darold.net> wrote: > Le 08/01/2026 à 08:43, Japin Li a écrit : >> On Thu, 08 Jan 2026 at 07:04, Gilles Darold <gilles@darold.net> wrote: >>> Le 08/01/2026 à 04:37, Japin Li a écrit : >>>> On Thu, 08 Jan 2026 at 10:57, "songjinzhou" <tsinghualucky912@foxmail.com> wrote: >>>>> Hi, Gilles Darold >>>>> >>>>> First of all, thank you for your reply. This is indeed not a simple >>>>> countdown. I did think it would be abrupt for users to see "0 days". I >>>>> checked v4, and I think it's fine. (PS: Should we add relevant >>>>> explanations to the SGML?) Thank you. >>>>> >>>> I'd like to hear more opinions on this. >>>> >>> Here is a new version of the patch that adds the documentation for the >>> new GUC, fix the warning message (days(s) instead of days) and handle >>> the 'Infinity' value for the VALID UNTIL clause. >>> >>> >> Thanks for updating the patch. >> >> 1. >> I noticed a warning when applying the patch. >> >> Applying: Add password_expire_warning GUC to warn clients >> .git/rebase-apply/patch:31: trailing whitespace. >> disable this behavior. The default value is <literal>7d</literal>. >> warning: 1 line adds whitespace errors. >> >> 2. >> <varlistentry id="guc-password-encryption" xreflabel="password_encryption"> >> - <term><varname>password_encryption</varname> (<type>enum</type>) >> + <term><varname>password_encryption</varname> (<type>enum</type> >> +) >> >> I think this modification isn't necessary. >> >> 3. >> + float8 result; >> + >> + result = ((float8) (vuntil - GetCurrentTimestamp())) / 1000000.0; /* in seconds */ >> + >> >> Perhaps we could use TimestampTz for the result variable and substitute >> USECS_PER_SEC for 1000000.0—that would avoid the unnecessary type cast. >> >> 4. >> + if ((int) result <= password_expire_warning) >> >> If the result exceeds INT_MAX, it triggers undefined behavior (IIRC). >> Therefore, we should probably cast password_expire_warning itself. >> >> 5. >> With this feature, GetCurrentTimestamp() might end up being called twice. >> Perhaps we can avoid that duplication. >> >> >> Attached is v6 of the patch addressing the issues above. Please take a look. > > > Thanks Japin, the implementation is fully working using the > TimestampTz cast. > > > I've attached a new patch to fix documentation and comments reported > by liu xiaohui and create a commitfest entry : > https://commitfest.postgresql.org/patch/6381/ Every one involved in > the review should edit the commitfest entry. > A minor nitpick: 1. + <varlistentry id="guc-password-expire-warnings" xreflabel="password_expire_warning"> + <term><varname>password_expire_warning</varname> (<type>integer</type>) We should probably use guc-password-expire-warning as the ID, since the GUC is named password_expire_warning (singular). -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
-
Re: Pasword expiration warning
Gilles DAROLD <gilles@darold.net> — 2026-01-08T12:31:15Z
Le 08/01/2026 à 10:27, Japin Li a écrit : > We should probably use guc-password-expire-warning as the ID, since the GUC is > named password_expire_warning (singular). Patch updated. -- Gilles Darold http://www.darold.net/
-
Re: Pasword expiration warning
Japin Li <japinli@hotmail.com> — 2026-01-09T02:31:30Z
On Thu, 08 Jan 2026 at 13:31, Gilles Darold <gilles@darold.net> wrote: > Le 08/01/2026 à 10:27, Japin Li a écrit : >> We should probably use guc-password-expire-warning as the ID, since the GUC is >> named password_expire_warning (singular). > > Patch updated. > Thanks for updating the patch. I noticed that src/backend/libpq/crypt.c no longer needs "postmaster/postmaster.h", so I've removed it in v8. I've also added a TAP test for the new GUC parameter. The updated patch is attached. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
-
Re: Pasword expiration warning
Yuefei Shi <shiyuefei1004@gmail.com> — 2026-01-09T06:10:47Z
A few review comments for V8. === 1. + if (password_expire_warning > 0 && vuntil < PG_INT64_MAX) + { + TimestampTz result = (vuntil - now) / USECS_PER_SEC; /* in seconds */ + + if (result <= (TimestampTz) password_expire_warning) + MyClientConnectionInfo.warning_message = + psprintf("your password will expire in %d day(s)", (int) (result / 86400)); + } Please consider localization of the warning message. 2. typo fix a. `Controls how many time ...` should be `Controls how much time ...`. b. `Sets how many time before password expire to emit ...` should be `Sets how much time before password expires to emit ...` -
Re: Pasword expiration warning
Japin Li <japinli@hotmail.com> — 2026-01-09T07:12:26Z
On Fri, 09 Jan 2026 at 14:10, Yuefei Shi <shiyuefei1004@gmail.com> wrote: > A few review comments for V8. > > === > 1. > + if (password_expire_warning > 0 && vuntil < PG_INT64_MAX) > + { > + TimestampTz result = (vuntil - now) / USECS_PER_SEC; /* in seconds */ > + > + if (result <= (TimestampTz) password_expire_warning) > + MyClientConnectionInfo.warning_message = > + psprintf("your password will expire in %d day(s)", (int) (result / 86400)); > + } > Please consider localization of the warning message. > > 2. typo fix > a. `Controls how many time ...` should be `Controls how much time ...`. > b. `Sets how many time before password expire to emit ...` should be `Sets how much time before password expires to emit ...` Nice catch. Updated in v9. Please take to look. I've also replaced the magic constant 86400 with the SECS_PER_DAY macro and enclosed the statement in braces since it now spans multiple lines. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd. -
Re: Pasword expiration warning
Steven Niu <niushiji@gmail.com> — 2026-01-09T07:36:59Z
From: Japin Li <japinli@hotmail.com> Sent: Friday, January 09, 2026 15:12 To: Yuefei Shi <shiyuefei1004@gmail.com> Cc: Gilles Darold <gilles@darold.net>; songjinzhou <tsinghualucky912@foxmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>; Andrew Dunstan <andrew@dunslane.net>; Bossart, Nathan <bossartn@amazon.com>; Tom Lane <tgl@sss.pgh.pa.us>; liu xiaohui <liuxh.zj.cn@gmail.com> Subject: Re: Pasword expiration warning On Fri, 09 Jan 2026 at 14:10, Yuefei Shi <shiyuefei1004@gmail.com> wrote: > A few review comments for V8. > > === > 1. > + if (password_expire_warning > 0 && vuntil < PG_INT64_MAX) > + { > + TimestampTz result = (vuntil - now) / USECS_PER_SEC; /* in seconds */ > + > + if (result <= (TimestampTz) password_expire_warning) > + MyClientConnectionInfo.warning_message = > + psprintf("your password will expire in %d day(s)", (int) (result / 86400)); > + } > Please consider localization of the warning message. > > 2. typo fix > a. `Controls how many time ...` should be `Controls how much time ...`. > b. `Sets how many time before password expire to emit ...` should be `Sets how much time before password expires to emit ...` Nice catch. Updated in v9. Please take to look. I've also replaced the magic constant 86400 with the SECS_PER_DAY macro and enclosed the statement in braces since it now spans multiple lines. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd. ________________________________________ Hi, Jiapin, I reviewed the v9-0002-Add-TAP-test-for-password_expire_warning.patch and here are my comments: 1. I think we should add tow more cases. One case is for the feature is disbaled. And another is for no warning when >1d remaining. 2. The modification to pg_hba.conf is unnecessary as the default pg_hba.conf generated by initdb already allows local connections with appropriate methods. unlink($node->data_dir . '/pg_hba.conf'); $node->append_conf('pg_hba.conf', "local all all scram-sha-256"); 3. Make the expected string to be more exact. qr/your password will expire in/); --> qr/your password will expire in 1d/); Thanks, Steven -
Re: Pasword expiration warning
Japin Li <japinli@hotmail.com> — 2026-01-09T09:04:31Z
Hi, Steven Thanks for the review. On Fri, 09 Jan 2026 at 07:36, Steven Niu <niushiji@gmail.com> wrote: > Hi, Jiapin, > > I reviewed the v9-0002-Add-TAP-test-for-password_expire_warning.patch > and here are my comments: > > 1. I think we should add tow more cases. One case is for the feature is disbaled. And another is for no warning when >1d remaining. Add in v10. > 2. The modification to pg_hba.conf is unnecessary as the default pg_hba.conf generated by initdb already allows local connections with appropriate methods. > unlink($node->data_dir . '/pg_hba.conf'); > $node->append_conf('pg_hba.conf', "local all all scram-sha-256"); Yes, it allows local connections, but they are always in trust mode, so no password is required (or used). > 3. Make the expected string to be more exact. > qr/your password will expire in/); > --> > qr/your password will expire in 1d/); > Fixed. PFA. v10-0001 - No changes. v10-0002 - Address review comments. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd. -
Re: Pasword expiration warning
Gilles DAROLD <gilles@darold.net> — 2026-01-09T11:27:59Z
Le 09/01/2026 à 10:04, Japin Li a écrit : > Hi, Steven > > Thanks for the review. > > On Fri, 09 Jan 2026 at 07:36, Steven Niu <niushiji@gmail.com> wrote: >> Hi, Jiapin, >> >> I reviewed the v9-0002-Add-TAP-test-for-password_expire_warning.patch >> and here are my comments: >> >> 1. I think we should add tow more cases. One case is for the feature is disbaled. And another is for no warning when >1d remaining. > Add in v10. > >> 2. The modification to pg_hba.conf is unnecessary as the default pg_hba.conf generated by initdb already allows local connections with appropriate methods. >> unlink($node->data_dir . '/pg_hba.conf'); >> $node->append_conf('pg_hba.conf', "local all all scram-sha-256"); > Yes, it allows local connections, but they are always in trust mode, so no > password is required (or used). > >> 3. Make the expected string to be more exact. >> qr/your password will expire in/); >> --> >> qr/your password will expire in 1d/); >> > Fixed. PFA. > > v10-0001 - No changes. > v10-0002 - Address review comments. > Here is a v11 version of the patch. v11-0001 - fix a miss on the typo fixes ( s/expire/expires/ in GUC description ) and add your name in the authors list. v11-0002 - Add a test with Infinity in VALID UNTIL value. -- Gilles Darold http://hexacluster.ai/ -
Re: Pasword expiration warning
Japin Li <japinli@hotmail.com> — 2026-01-09T11:56:13Z
> Here is a v11 version of the patch. > > v11-0001 - fix a miss on the typo fixes ( s/expire/expires/ in GUC > description ) and add your name in the authors list. > > v11-0002 - Add a test with Infinity in VALID UNTIL value. > > Thanks for updating the patches. LGTM. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
-
Re: Pasword expiration warning
Soumya S Murali <soumyamurali.work@gmail.com> — 2026-01-28T05:23:24Z
Hi all, Thank you for the updated patches. On Tue, Jan 27, 2026 at 12:21 PM Gilles Darold <gilles@darold.net> wrote: > > Le 09/01/2026 à 10:04, Japin Li a écrit : > > Hi, Steven > > > > Thanks for the review. > > > > On Fri, 09 Jan 2026 at 07:36, Steven Niu <niushiji@gmail.com> wrote: > >> Hi, Jiapin, > >> > >> I reviewed the v9-0002-Add-TAP-test-for-password_expire_warning.patch > >> and here are my comments: > >> > >> 1. I think we should add tow more cases. One case is for the feature is disbaled. And another is for no warning when >1d remaining. > > Add in v10. > > > >> 2. The modification to pg_hba.conf is unnecessary as the default pg_hba.conf generated by initdb already allows local connections with appropriate methods. > >> unlink($node->data_dir . '/pg_hba.conf'); > >> $node->append_conf('pg_hba.conf', "local all all scram-sha-256"); > > Yes, it allows local connections, but they are always in trust mode, so no > > password is required (or used). > > > >> 3. Make the expected string to be more exact. > >> qr/your password will expire in/); > >> --> > >> qr/your password will expire in 1d/); > >> > > Fixed. PFA. > > > > v10-0001 - No changes. > > v10-0002 - Address review comments. > > > > Here is a v11 version of the patch. > > v11-0001 - fix a miss on the typo fixes ( s/expire/expires/ in GUC > description ) and add your name in the authors list. > > v11-0002 - Add a test with Infinity in VALID UNTIL value. I went through the discussions and I applied the posted patches on the current master branch and have completed testing. Firstly, the conceptual approach of adding a server-side password_expire_warning GUC in patch 0001 looks reasonable for me too as it allows all clients to benefit from the warning. Here, the password expiry enforcement is strictly tied to password-based authentication. With md5 authentication explicitly configured, expiry enforcement works as expected, login succeeds while the password is valid and fails with “password has expired” once the expiry timestamp is reached. This is confirmed via server logs showing the md5 authentication path being exercised. When non-password authentication methods (trust/peer) are used, password expiry is bypassed. While expiry enforcement functions correctly after expiry, no advance warning is emitted prior to expiry in the baseline behavior, which matches the motivation for this change. The approach in patch 0001 of adding a server-side password_expire_warning GUC and adding the corresponding TAP coverage in patch 0002 seems directionally correct. Regards, Soumya -
Re: Pasword expiration warning
Euler Taveira <euler@eulerto.com> — 2026-01-28T12:44:55Z
On Fri, Jan 9, 2026, at 8:27 AM, Gilles Darold wrote: > > Here is a v11 version of the patch. > + if (result <= (TimestampTz) password_expire_warning) + { + MyClientConnectionInfo.warning_message = + psprintf(_("your password will expire in %d day(s)"), + (int) (result / SECS_PER_DAY)); + } You should use ngettext() for plural forms. I don't think you need a string into ClientConnectionInfo. Instead, you could store only the number of days. + /* + * Emit a warning message to the client when set, for example + * to warn the user that the password will expire. + */ + if (MyClientConnectionInfo.warning_message) + ereport(WARNING, (errmsg("%s", MyClientConnectionInfo.warning_message))); + ... and you construct the message directly in the ereport(). -- Euler Taveira EDB https://www.enterprisedb.com/ -
Re: Pasword expiration warning
Zsolt Parragi <zsolt.parragi@percona.com> — 2026-01-28T19:25:34Z
Hello! A first question: have you looked at the GoAway patch[1]? While that isn't exactly about the same situation, it was already considered for password expiration checks in[2], and the same idea could be useful for this situation too, especially in the context of my last question in this email. + MyClientConnectionInfo.warning_message = + psprintf(_("your password will expire in %d day(s)"), + (int) (result / SECS_PER_DAY)); Shouldn't that use MemoryContextStrtup(TopMemoryContext, ...)? + /* + * Message to send to the client in case of connection success. + * When not NULL a WARNING message is sent to the client at end + * of the connection in src/backend/utils/init/postinit.c at + * enf of InitPostgres(). For example, it is use to show the + * password expiration warning. + */ + const char *warning_message; Handling of this new variable is missing from EstimateClientConnectionInfoSpace and SerializeClientConnectionInfo, which the struct explicitly asks for a few lines above this change. Even if you think that's not necessary for some reason, it should be explained to avoid confusing readers. + * Password OK, but check if rolvaliduntil is less than GUC + * password_expire_warning days to send a warning to the client + */ + if (!isnull && password_expire_warning > 0 && vuntil < PG_INT64_MAX) Could this use TIMESTAMP_NOT_FINITE? And I think that "days" should be "seconds". + TimestampTz result = (vuntil - now) / USECS_PER_SEC; /* in seconds */ Maybe call this variable something more descriptive, like seconds_until_expiration? + + if (result <= (TimestampTz) password_expire_warning) + { + MyClientConnectionInfo.warning_message = + psprintf(_("your password will expire in %d day(s)"), + (int) (result / SECS_PER_DAY)); + } This is not that useful on the last day - have you considered displaying hours if the expiration date is within a day, or maybe HH:MM? There are a few typos (warnin in the commit message, disable -> disables in the documentation, enf -> end in a comment) I would also consider long lived connections. The warning currently only fires when a user connects, maybe it would be useful to also do something when the user enters the expiration period during an active connection? [1]: https://www.postgresql.org/message-id/DDPQ1RV5FE9U.I2WW34NGRD8Z%40jeltef.nl [2]: https://www.postgresql.org/message-id/CAER375OvH3_ONmc-SgUFpA6gv_d6eNj2KdZktzo-f_uqNwwWNw%40mail.gmail.com -
Re: Pasword expiration warning
Japin Li <japinli@hotmail.com> — 2026-01-29T11:44:33Z
On Wed, 28 Jan 2026 at 19:25, Zsolt Parragi <zsolt.parragi@percona.com> wrote: > Hello! > > A first question: have you looked at the GoAway patch[1]? While that > isn't exactly about the same situation, it was already considered for > password expiration checks in[2], and the same idea could be useful > for this situation too, especially in the context of my last question > in this email. I didn't follow the thread. If we perform password expiration checks based on an interval, wouldn’t it also be possible to emit the warning during that check? > > + MyClientConnectionInfo.warning_message = > + psprintf(_("your password will expire in %d day(s)"), > + (int) (result / SECS_PER_DAY)); > > Shouldn't that use MemoryContextStrtup(TopMemoryContext, ...)? Here the memory context is TopTransactionContext. I don't think this would cause any problem. Could you explain why you prefer TopMemoryContext? > > + /* > + * Message to send to the client in case of connection success. > + * When not NULL a WARNING message is sent to the client at end > + * of the connection in src/backend/utils/init/postinit.c at > + * enf of InitPostgres(). For example, it is use to show the > + * password expiration warning. > + */ > + const char *warning_message; > > Handling of this new variable is missing from > EstimateClientConnectionInfoSpace and SerializeClientConnectionInfo, > which the struct explicitly asks for a few lines above this change. > Even if you think that's not necessary for some reason, it should be > explained to avoid confusing readers. > > + * Password OK, but check if rolvaliduntil is less than GUC > + * password_expire_warning days to send a warning to the client > + */ > + if (!isnull && password_expire_warning > 0 && vuntil < PG_INT64_MAX) > > Could this use TIMESTAMP_NOT_FINITE? > > And I think that "days" should be "seconds". > > + TimestampTz result = (vuntil - now) / USECS_PER_SEC; /* in seconds */ > > Maybe call this variable something more descriptive, like > seconds_until_expiration? > > + > + if (result <= (TimestampTz) password_expire_warning) > + { > + MyClientConnectionInfo.warning_message = > + psprintf(_("your password will expire in %d day(s)"), > + (int) (result / SECS_PER_DAY)); > + } > > This is not that useful on the last day - have you considered > displaying hours if the expiration date is within a day, or maybe > HH:MM? > Yeah, this was already proposed in [0], but there was no consensus. > There are a few typos (warnin in the commit message, disable -> > disables in the documentation, enf -> end in a comment) > Good catch. > I would also consider long lived connections. The warning currently > only fires when a user connects, maybe it would be useful to also do > something when the user enters the expiration period during an active > connection? > > [1]: https://www.postgresql.org/message-id/DDPQ1RV5FE9U.I2WW34NGRD8Z%40jeltef.nl > [2]: https://www.postgresql.org/message-id/CAER375OvH3_ONmc-SgUFpA6gv_d6eNj2KdZktzo-f_uqNwwWNw%40mail.gmail.com [0] https://www.postgresql.org/message-id/tencent_8C7890411E85257F512EFE4F0DD00EE29806%40qq.com -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd. -
Re: Pasword expiration warning
Zsolt Parragi <zsolt.parragi@percona.com> — 2026-01-29T13:41:05Z
> Yeah, this was already proposed in [0], but there was no consensus. Sorry, I missed that! I should have read all earlier messages in more detail. > Here the memory context is TopTransactionContext. I don't think this would > cause any problem. Could you explain why you prefer TopMemoryContext? I mainly looked at it similarly to the serialization question: how existing parts of the struct are used? There are two places where authn_id, the other string field, is set. Both use the same pattern, and if I read the code correctly one of them is already in TopMemoryContext, similarly to this call place. (and the other context is the parallel worker context, which would also be long lived enough for storing the authn_field, so again, technically the explicit TopMemoryContext isn't necessary) It seems to me that both existing places only use this explicit way of allocating it in TopMemoryContext for clarity.
-
Re: Pasword expiration warning
Gilles DAROLD <gilles@darold.net> — 2026-01-29T15:39:54Z
Le 28/01/2026 à 13:44, Euler Taveira a écrit : > On Fri, Jan 9, 2026, at 8:27 AM, Gilles Darold wrote: >> Here is a v11 version of the patch. >> > + if (result <= (TimestampTz) password_expire_warning) > + { > + MyClientConnectionInfo.warning_message = > + psprintf(_("your password will expire in %d day(s)"), > + (int) (result / SECS_PER_DAY)); > + } > > You should use ngettext() for plural forms. Is it a use we must do now or a wish? $ grep -r "ngettext(" src/backend/ | wc -l 9 $ grep -r "(s)" src/backend/ | wc -l 831 If this is not a must do now, I prefer to use the old way because we don't have to repeat 2 times the constant. > I don't think you need a string into ClientConnectionInfo. Instead, you could store only the number of days. > > + /* > + * Emit a warning message to the client when set, for example > + * to warn the user that the password will expire. > + */ > + if (MyClientConnectionInfo.warning_message) > + ereport(WARNING, (errmsg("%s", MyClientConnectionInfo.warning_message))); > + > > ... and you construct the message directly in the ereport(). I have explained this choice in my first message: "I have chosen to add a new field, const char *warning_message, to struct ClientConnectionInfo so that it can be used to send other messages to the client at end of connection ( src/backend/utils/init/postinit.c: InitPostgres() ). Not sure sure that this is the best way to do that but as it is a message dedicated to the connection I've though it could be the right place. If we don't expect other warning message sent to the client at connection time, just using an integer for the number of days remaining will be enough. We could use notice but it is not logged by default and also I think that warning is the good level for this message. " -- Gilles Darold http://hexacluster.ai/ -
Re: Pasword expiration warning
Gilles DAROLD <gilles@darold.net> — 2026-01-29T16:07:04Z
Le 28/01/2026 à 20:25, Zsolt Parragi a écrit : > Hello! > > A first question: have you looked at the GoAway patch[1]? While that > isn't exactly about the same situation, it was already considered for > password expiration checks in[2], and the same idea could be useful > for this situation too, especially in the context of my last question > in this email. I don't know about this thread before you mention it. With a quick read of the thread it looks that this GoAway protocol addition is use to ask to the client to disconnect/reconnect. Here we just want to emit a warning at connection to inform the user that his password will expire and it don't need re-connection at all. Anyway I will have a deeper look in this thread. > + /* > + * Message to send to the client in case of connection success. > + * When not NULL a WARNING message is sent to the client at end > + * of the connection in src/backend/utils/init/postinit.c at > + * enf of InitPostgres(). For example, it is use to show the > + * password expiration warning. > + */ > + const char *warning_message; > > Handling of this new variable is missing from > EstimateClientConnectionInfoSpace and SerializeClientConnectionInfo, > which the struct explicitly asks for a few lines above this change. > Even if you think that's not necessary for some reason, it should be > explained to avoid confusing readers. This is intentional because this message is only emitted at the main connection and don't needed to be in the MyClientConnectionInfo serialization. I forgot to add a comment, I will do. > + * Password OK, but check if rolvaliduntil is less than GUC > + * password_expire_warning days to send a warning to the client > + */ > + if (!isnull && password_expire_warning > 0 && vuntil < PG_INT64_MAX) > > Could this use TIMESTAMP_NOT_FINITE? Thanks, it will be fixed too. > And I think that "days" should be "seconds". > > + TimestampTz result = (vuntil - now) / USECS_PER_SEC; /* in seconds */ > > Maybe call this variable something more descriptive, like > seconds_until_expiration? > > + > + if (result <= (TimestampTz) password_expire_warning) > + { > + MyClientConnectionInfo.warning_message = > + psprintf(_("your password will expire in %d day(s)"), > + (int) (result / SECS_PER_DAY)); > + } > > This is not that useful on the last day - have you considered > displaying hours if the expiration date is within a day, or maybe > HH:MM? When you see that the password is about to expire in 0 day, do you really think that saying it will expire in 12h30m42s will encourage the user to change it now? If he don't do that in the previous days he will probably not do it in the hour too. Quite useless IMO but if there more vote to have HH:MM why not. -- Gilles Darold http://hexacluster.ai/ -
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-01-29T17:53:08Z
Sorry, I haven't been following the discussion, but I took a brief look at the latest patch in the thread. + Controls how much time (in seconds) before a role's password expiration + a <literal>WARNING</literal> message is sent to the client upon successful + connection. It requires that a <command>VALID UNTIL</command> date is set + for the role. A value of <literal>0d</literal> disable this behavior. The + default value is <literal>7d</literal> and the maximum value <literal>30d</literal>. I'm not sure we should subject folks to these warnings by default, and I don't see a reason to restrict the maximum value to 30 days. IMHO we should have this disabled by default and the maximum value should be INT_MAX. + if (password_expire_warning > 0 && vuntil < PG_INT64_MAX) + { + TimestampTz result = (vuntil - now) / USECS_PER_SEC; /* in seconds */ + + if (result <= (TimestampTz) password_expire_warning) + { + MyClientConnectionInfo.warning_message = + psprintf(_("your password will expire in %d day(s)"), + (int) (result / SECS_PER_DAY)); + } + } nitpick: I suspect we could simplify this code a bit, but I haven't tried. Also, IMO we should be more precise about the expiration time. There is a reasonable difference between a password expiring in 1 second as opposed to 23 hours, 59 minutes, 59 seconds, but in both cases this message would say "0 days". You might be able to borrow from psql/common.c's PrintTiming() function to add more detail here. + /* + * Emit a warning message to the client when set, for example + * to warn the user that the password will expire. + */ + if (MyClientConnectionInfo.warning_message) + ereport(WARNING, (errmsg("%s", MyClientConnectionInfo.warning_message))); Having a variable for warning messages could come in handy later. For example, we might add a warning about using MD5 passwords at some point. In my draft patch for this [0], I put the warning after closing the transaction, whereas this patch puts it just before. I'm not sure I had a principled reason for doing so, but it's an interesting difference between the two patches. [0] https://postgr.es/m/attachment/177167/v2-0002-WIP-add-warning-upon-authentication-with-MD5-pass.patch -- nathan -
Re: Pasword expiration warning
Zsolt Parragi <zsolt.parragi@percona.com> — 2026-01-29T20:44:21Z
> When you see that the password is about to expire in 0 day, do you > really think that saying it will expire in 12h30m42s will encourage the > user to change it now? If he don't do that in the previous days he will > probably not do it in the hour too. Quite useless IMO but if there more > vote to have HH:MM why not. My concern with this is mainly the relation with the 2 threads I linked. The password expiration patch disconnects users after their password expires, opposed to the current behavior of letting existing connections to continue - which I think is a quite useful security improvement. And with that, the exact expiration time, maybe even periodic reminders while the connection is active are way more useful. ("Your password is only valid for 2 more hours, please don't forget this or you will be disconnected" ... "Now you only have 15 minutes, last chance to fix it") This is why I think something that could make periodic reminders, not only one reminder during when the client connects, could be useful. Even if not GoAway itself, maybe something similar to it? I mainly linked these because I think the goal/problem is similar, and while both patches look good separately, the user experience could use some improvements if both get merged. (Another related discussion in the password expiration thread is oauth token expiration checks, which could use similar "Your token expired, you have X more minutes or you will be disconnected" messages) -
Re: Pasword expiration warning
Gilles DAROLD <gilles@darold.net> — 2026-01-29T21:48:39Z
Le 29/01/2026 à 18:53, Nathan Bossart a écrit : > Sorry, I haven't been following the discussion, but I took a brief look at > the latest patch in the thread. > > + Controls how much time (in seconds) before a role's password expiration > + a <literal>WARNING</literal> message is sent to the client upon successful > + connection. It requires that a <command>VALID UNTIL</command> date is set > + for the role. A value of <literal>0d</literal> disable this behavior. The > + default value is <literal>7d</literal> and the maximum value <literal>30d</literal>. > > I'm not sure we should subject folks to these warnings by default, and I > don't see a reason to restrict the maximum value to 30 days. IMHO we > should have this disabled by default and the maximum value should be > INT_MAX. This was my first though but I agree with Tom comment "Off-by-default is pretty much guaranteed to not help most people.". I will use INT_MAX. > + if (password_expire_warning > 0 && vuntil < PG_INT64_MAX) > + { > + TimestampTz result = (vuntil - now) / USECS_PER_SEC; /* in seconds */ > + > + if (result <= (TimestampTz) password_expire_warning) > + { > + MyClientConnectionInfo.warning_message = > + psprintf(_("your password will expire in %d day(s)"), > + (int) (result / SECS_PER_DAY)); > + } > + } > > nitpick: I suspect we could simplify this code a bit, but I haven't tried. > > Also, IMO we should be more precise about the expiration time. There is a > reasonable difference between a password expiring in 1 second as opposed to > 23 hours, 59 minutes, 59 seconds, but in both cases this message would say > "0 days". You might be able to borrow from psql/common.c's PrintTiming() > function to add more detail here. Ok, there's now more vote for being more precise about the expiration time, I will add it. > + /* > + * Emit a warning message to the client when set, for example > + * to warn the user that the password will expire. > + */ > + if (MyClientConnectionInfo.warning_message) > + ereport(WARNING, (errmsg("%s", MyClientConnectionInfo.warning_message))); > > Having a variable for warning messages could come in handy later. For > example, we might add a warning about using MD5 passwords at some point. > In my draft patch for this [0], I put the warning after closing the > transaction, whereas this patch puts it just before. I'm not sure I had a > principled reason for doing so, but it's an interesting difference between > the two patches. > > [0] https://postgr.es/m/attachment/177167/v2-0002-WIP-add-warning-upon-authentication-with-MD5-pass.patch Understood, I will rewrite the patch to use a int variable. Thanks. -- Gilles Darold http://www.darold.net/ -
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-01-29T21:56:25Z
On Thu, Jan 29, 2026 at 10:48:39PM +0100, Gilles Darold wrote: > Le 29/01/2026 à 18:53, Nathan Bossart a écrit : >> Having a variable for warning messages could come in handy later. For >> example, we might add a warning about using MD5 passwords at some point. >> In my draft patch for this [0], I put the warning after closing the >> transaction, whereas this patch puts it just before. I'm not sure I had a >> principled reason for doing so, but it's an interesting difference between >> the two patches. > > Understood, I will rewrite the patch to use a int variable. Sorry if my note was not clear, but I didn't mean to suggest rewriting anything here. I thought the difference in placement for the warning between your patch and mine was interesting, but I'm not sure there's anything wrong that needs to be changed. -- nathan
-
Re: Pasword expiration warning
Gilles DAROLD <gilles@darold.net> — 2026-01-30T11:33:54Z
Here a new v12 version of the patch. Changes are the following: - Add more precision to the time remaining with format '%d day(s) %02dh%02dm'. - Change GUC maximum value limit from 30d to INT_MAX - Add comment to struct SerializedClientConnectionInfo explaining why the warning_message variable is not reported from struct ClientConnectionInfo. - Use TIMESTAMP_INFINITY instead of PG_INT64_MAX to check Infinity value for rolvaliduntil. - Fix 2 typo. - Update TAP tests to reflect the time format change. -- Gilles Darold http://hexacluster.ai/
-
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-02-02T17:04:03Z
On Fri, Jan 30, 2026 at 12:33:54PM +0100, Gilles Darold wrote: > Here a new v12 version of the patch. Changes are the following: Thanks. I spent some time preparing this for commit, and I came up with the attached. Notable changes include: * Renamed the parameter to password_expiration_warning_threshold. It's a mouthful, but I thought it was more descriptive. * Changed the units for the parameter to minutes. I can't imagine anyone needs more granularity than hours or days, let alone seconds, so IMO minutes is a good middle ground. * I added a new "connection warnings" infrastructure that we can reuse if/when we want to emit warnings for MD5 passwords. * Moved the warning messages to Port. ClientConnectionInfo appears to be meant only for parallel workers, and I don't think we will ever want to emit connection warnings there. * Moved the tests into 001_password.pl. I'm a bit concerned about these tests being flaky, but I've tried setting the VALID UNTIL dates to make spurious failures virtually impossible. WDYT? -- nathan
-
Re: Pasword expiration warning
Gilles DAROLD <gilles@darold.net> — 2026-02-02T17:43:28Z
Le 02/02/2026 à 18:04, Nathan Bossart a écrit : > On Fri, Jan 30, 2026 at 12:33:54PM +0100, Gilles Darold wrote: >> Here a new v12 version of the patch. Changes are the following: > Thanks. I spent some time preparing this for commit, and I came up with > the attached. Notable changes include: > > * Renamed the parameter to password_expiration_warning_threshold. It's a > mouthful, but I thought it was more descriptive. > > * Changed the units for the parameter to minutes. I can't imagine anyone > needs more granularity than hours or days, let alone seconds, so IMO > minutes is a good middle ground. > > * I added a new "connection warnings" infrastructure that we can reuse > if/when we want to emit warnings for MD5 passwords. > > * Moved the warning messages to Port. ClientConnectionInfo appears to be > meant only for parallel workers, and I don't think we will ever want to > emit connection warnings there. > > * Moved the tests into 001_password.pl. I'm a bit concerned about these > tests being flaky, but I've tried setting the VALID UNTIL dates to make > spurious failures virtually impossible. > > WDYT? Thanks Nathan, I'm comfortable with all the changes except the first two. I think configuration name password_expiration_warning_threshold is too long, why not only password_warning_threshold? I think the description is here to explain more the configuration directive. About the unit, I think using minutes is useless. It should be set in days to be really useful but like all other time based directives it can be set in seconds, minutes or hours too. This give the user the granularity he wants. The common use will be to set it using 'Nd' syntax but if someone wants to use 'Ns' syntax, it can be done. -- Gilles Darold http://www.darold.net/
-
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-02-02T17:55:14Z
On Mon, Feb 02, 2026 at 06:43:28PM +0100, Gilles Darold wrote: > I think configuration name password_expiration_warning_threshold is too > long, why not only password_warning_threshold? I think the description is > here to explain more the configuration directive. I agree that it's long, but leaving out "expiration" makes the name rather ambiguous. FWIW we do have 3 other GUCs with lengths >= this (max_parallel_apply_workers_per_subscription, ssl_passphrase_command_supports_reload, and autovacuum_vacuum_insert_scale_factor). > About the unit, I think using minutes is useless. It should be set in days > to be really useful but like all other time based directives it can be set > in seconds, minutes or hours too. This give the user the granularity he > wants. The common use will be to set it using 'Nd' syntax but if someone > wants to use 'Ns' syntax, it can be done. Not all time-based GUCs use seconds. log_rotation_age and wal_summary_keep_time use minutes, and many others using milliseconds. But I'm fine with changing it back to seconds. That would still allow setting the threshold up to ~68 years, which ought to be enough for anyone. -- nathan
-
Re: Pasword expiration warning
Zsolt Parragi <zsolt.parragi@percona.com> — 2026-02-02T20:25:42Z
Hello! In the commit message: > This commit adds a new parameter called > parameter_expiration_warning_threshold that controls when the That should be password expiration warning It is also clearer to me with expiration in it, without that I would think about a complexity warning first. And in the documentation: + default is 7 days. This parameter can only set in the That should be "can only be set"
-
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-02-02T20:31:02Z
Here is an updated patch with the units set to seconds. There are two main things on my mind: * The placement of the WARNING. Right now, I have it placed at the end of InitPostgres(). There are various other ways to get a WARNING during this function, so I think it's technically okay, but perhaps it makes more sense to put it at the end of ClientAuthentication() or something. But the risk there is that something between the call to ClientAuthentication() and the end of InitPostgres() could ERROR/FATAL, in which case our new WARNING might be giving away more information than necessary. So, I guess I lean towards keeping it where it is now, but I would be interested to hear other opinions on the matter. * Whether we should emit the warnings for special client backends. Specifically, I think the current patch will send warnings to logical replication connections, but not physical replication connections. My current feeling is that we should send warnings to any backend that uses a password to authenticate, i.e., add a call to EmitConnectionWarnings() at the end of the "am_walsender && !am_db_walsender" block. Thoughts? Are there any other backend types I'm forgetting that would be relevant here? -- nathan
-
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-02-03T15:28:49Z
On Mon, Feb 02, 2026 at 02:31:02PM -0600, Nathan Bossart wrote: > Here is an updated patch with the units set to seconds. There are two main > things on my mind: > > * The placement of the WARNING. Right now, I have it placed at the end of > InitPostgres(). There are various other ways to get a WARNING during this > function, so I think it's technically okay, but perhaps it makes more sense > to put it at the end of ClientAuthentication() or something. But the risk > there is that something between the call to ClientAuthentication() and the > end of InitPostgres() could ERROR/FATAL, in which case our new WARNING > might be giving away more information than necessary. So, I guess I lean > towards keeping it where it is now, but I would be interested to hear other > opinions on the matter. > > * Whether we should emit the warnings for special client backends. > Specifically, I think the current patch will send warnings to logical > replication connections, but not physical replication connections. My > current feeling is that we should send warnings to any backend that uses a > password to authenticate, i.e., add a call to EmitConnectionWarnings() at > the end of the "am_walsender && !am_db_walsender" block. Thoughts? Are > there any other backend types I'm forgetting that would be relevant here? Hearing nothing, I've updated the patch to send warnings to all backends that use a password to authenticate. -- nathan
-
Re: Pasword expiration warning
Greg Sabino Mullane <htamfids@gmail.com> — 2026-02-03T17:08:08Z
On Tue, Feb 3, 2026 at 10:29 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > Hearing nothing, I've updated the patch to send warnings to all backends > that use a password to authenticate. > +1, that makes sense. Cheers, Greg -- Crunchy Data - https://www.crunchydata.com Enterprise Postgres Software Products & Tech Support
-
Re: Pasword expiration warning
Peter Eisentraut <peter@eisentraut.org> — 2026-02-04T14:44:05Z
On 03.02.26 16:28, Nathan Bossart wrote: > + detail = psprintf(_("The password for role \"%s\" will expire in " > + INT64_FORMAT " day(s), " INT64_FORMAT > + " hour(s), " INT64_FORMAT " minute(s)."), > + role, days, hours, minutes); You cannot use INT64_FORMAT inside translatable messages. But you can use PRId64. Using the type TimestampTz for what are essentially interval/duration quantities is a bit weird and confusing. So maybe another placeholder would be more appropriate. That said, I find writing plurals with "(s)" kind of lame. It's not a good look. It's a bit difficult to do this correctly when you have three separate values in one string. I would consider for example just showing the number of days if the value is larger than one day, number hours if it's larger than one hour, else minutes. I don't think you need minute-precision when the expiration time is several days out. Alternatively, just print the actual expiration timestamp. -
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-02-04T16:42:59Z
On Wed, Feb 04, 2026 at 03:44:05PM +0100, Peter Eisentraut wrote: > On 03.02.26 16:28, Nathan Bossart wrote: >> + detail = psprintf(_("The password for role \"%s\" will expire in " >> + INT64_FORMAT " day(s), " INT64_FORMAT >> + " hour(s), " INT64_FORMAT " minute(s)."), >> + role, days, hours, minutes); > > You cannot use INT64_FORMAT inside translatable messages. But you can use > PRId64. Ah, I didn't know that. Is this documented anywhere? I skimmed through our message-writing guidelines [0] but didn't see any mention of this. > Using the type TimestampTz for what are essentially interval/duration > quantities is a bit weird and confusing. So maybe another placeholder would > be more appropriate. Sure. > That said, I find writing plurals with "(s)" kind of lame. It's not a good > look. > > It's a bit difficult to do this correctly when you have three separate > values in one string. I would consider for example just showing the number > of days if the value is larger than one day, number hours if it's larger > than one hour, else minutes. I don't think you need minute-precision when > the expiration time is several days out. I think we still have the problem with plurals if we go this route, and IIUC there isn't a good way to do something like errmsg_plural() here since we are just saving the message for later. I'm skeptical it's worth adding new translatable-plural handling functionality for this. > Alternatively, just print the actual expiration timestamp. That crossed my mind, but I worried about timezone/formatting questions, and I haven't found any examples of putting a timestamp in an error message like this. Is it acceptable to use pg_strftime() in a translated string? [0] https://www.postgresql.org/docs/devel/nls-programmer.html#NLS-GUIDELINES -- nathan -
Re: Pasword expiration warning
Zsolt Parragi <zsolt.parragi@percona.com> — 2026-02-04T17:44:06Z
> > Alternatively, just print the actual expiration timestamp. > > That crossed my mind, but I worried about timezone/formatting questions, > and I haven't found any examples of putting a timestamp in an error message > like this. Is it acceptable to use pg_strftime() in a translated string? Wouldn't that be more confusing? The password for role "foo" will expire at 2026-02-05 16:56" No matter which date format is used, this is harder to understand than The password for role "foo" will expire in 1 day(s) or The password for role "foo" will expire in 22 hour(s) 16 minute(s) All 3 are about a password expiring around the same time plus minus a few hours, but the first one is significantly harder to figure out, first I have to think about what's the exact date today.
-
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-02-04T20:22:29Z
On Wed, Feb 04, 2026 at 05:44:06PM +0000, Zsolt Parragi wrote: > Wouldn't that be more confusing? > > The password for role "foo" will expire at 2026-02-05 16:56" > > No matter which date format is used, this is harder to understand than > > The password for role "foo" will expire in 1 day(s) Fair enough. The main problem we need to figure out for that approach is how to handle plurals without "(s)". One option is to move the ereport() to this function, but as before [0], I'm not sure we want to send a warning before more of the initialization has completed. Another option could be to add a new function that checks the expiration time and emits a warning as needed, and call that at the end of InitPostgres(). [0] https://postgr.es/m/aYEJhk83XAqk76dP%40nathan -- nathan
-
Re: Pasword expiration warning
Jacob Champion <jacob.champion@enterprisedb.com> — 2026-02-04T20:56:18Z
On Wed, Feb 4, 2026 at 12:22 PM Nathan Bossart <nathandbossart@gmail.com> wrote: > Fair enough. The main problem we need to figure out for that approach is > how to handle plurals without "(s)". Euler pointed out upthread that the _() call should be replaced with ngettext(). Or is there a reason you need the more complicated errmsg_plural() machinery? --Jacob
-
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-02-04T21:00:03Z
On Wed, Feb 04, 2026 at 12:56:18PM -0800, Jacob Champion wrote: > On Wed, Feb 4, 2026 at 12:22 PM Nathan Bossart <nathandbossart@gmail.com> wrote: >> Fair enough. The main problem we need to figure out for that approach is >> how to handle plurals without "(s)". > > Euler pointed out upthread that the _() call should be replaced with > ngettext(). Or is there a reason you need the more complicated > errmsg_plural() machinery? Ah, that's what I was looking for, thanks. I missed that earlier message. -- nathan
-
Re: Pasword expiration warning
Euler Taveira <euler@eulerto.com> — 2026-02-04T21:12:07Z
On Wed, Feb 4, 2026, at 1:42 PM, Nathan Bossart wrote: > On Wed, Feb 04, 2026 at 03:44:05PM +0100, Peter Eisentraut wrote: >> On 03.02.26 16:28, Nathan Bossart wrote: >>> + detail = psprintf(_("The password for role \"%s\" will expire in " >>> + INT64_FORMAT " day(s), " INT64_FORMAT >>> + " hour(s), " INT64_FORMAT " minute(s)."), >>> + role, days, hours, minutes); >> >> You cannot use INT64_FORMAT inside translatable messages. But you can use >> PRId64. > > Ah, I didn't know that. Is this documented anywhere? I skimmed through > our message-writing guidelines [0] but didn't see any mention of this. > In the gettext documentation [1]. Maybe we should also add this information to the referred section. >> That said, I find writing plurals with "(s)" kind of lame. It's not a good >> look. >> >> It's a bit difficult to do this correctly when you have three separate >> values in one string. I would consider for example just showing the number >> of days if the value is larger than one day, number hours if it's larger >> than one hour, else minutes. I don't think you need minute-precision when >> the expiration time is several days out. > > I think we still have the problem with plurals if we go this route, and > IIUC there isn't a good way to do something like errmsg_plural() here since > we are just saving the message for later. I'm skeptical it's worth adding > new translatable-plural handling functionality for this. > 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. >> Alternatively, just print the actual expiration timestamp. > > That crossed my mind, but I worried about timezone/formatting questions, > and I haven't found any examples of putting a timestamp in an error message > like this. Is it acceptable to use pg_strftime() in a translated string? > Although we can add the expiration timestamp, I don't think that's a good UI because (a) there might be timezone difference and (b) you need to do the math of the remaining time. We can add it but when we are talking about time until a certain event, we are expecting a countdown format. If the user decides to change the password because of this message, we don't need inform the exact remaining time. I agree with Peter that just the largest unit is sufficient for the user to take some action. Let's say the remaining time is 1 day and 14 hours. If I just inform the 1 day, I know that I have at least 24 hours to change it and it is sufficient to postpone that action to tomorrow because your work day ends in a few minutes. [1] https://www.gnu.org/software/gettext/manual/html_node/No-string-concatenation.html#The-_003cinttypes_002eh_003e-macros [2] https://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html -- Euler Taveira EDB https://www.enterprisedb.com/ -
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-02-04T22:15:58Z
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
-
Re: Pasword expiration warning
Euler Taveira <euler@eulerto.com> — 2026-02-05T00:51:10Z
On Wed, Feb 4, 2026, at 7:15 PM, Nathan Bossart 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. > LGTM. Translation works fine. $ psql -U baz Senha para o usuário baz: AVISO: senha expirará em breve DETALHE: A senha da role "baz" expirará em 3 dias. $ psql -U bar Senha para o usuário bar: AVISO: senha expirará em breve DETALHE: A senha da role "bar" expirará em 2 horas. $ psql -U bar1 Senha para o usuário bar1: AVISO: senha expirará em breve DETALHE: A senha da role "bar1" expirará em 15 minutos. $ psql -U bar2 Senha para o usuário bar2: AVISO: senha expirará em breve DETALHE: A senha da role "bar2" expirará em menos de 1 minuto. -- Euler Taveira EDB https://www.enterprisedb.com/
-
Re: Pasword expiration warning
Chao Li <li.evan.chao@gmail.com> — 2026-02-05T01:06:27Z
> On Feb 5, 2026, at 06:15, Nathan Bossart <nathandbossart@gmail.com> 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/ -
Re: Pasword expiration warning
Greg Sabino Mullane <htamfids@gmail.com> — 2026-02-05T14:39:53Z
On Wed, Feb 4, 2026 at 8:07 PM Chao Li <li.evan.chao@gmail.com> wrote: > * 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`. > -1. We allow very large GUC ranges for lots of things, and don't try to catch accidental settings. Also, 70d seems a perfectly reasonable value to me. -- Cheers, Greg
-
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-02-05T15:16:20Z
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 -
Re: Pasword expiration warning
Peter Eisentraut <peter@eisentraut.org> — 2026-02-05T18:42:22Z
On 04.02.26 18:44, Zsolt Parragi wrote: >>> Alternatively, just print the actual expiration timestamp. >> >> That crossed my mind, but I worried about timezone/formatting questions, >> and I haven't found any examples of putting a timestamp in an error message >> like this. Is it acceptable to use pg_strftime() in a translated string? > > Wouldn't that be more confusing? > > The password for role "foo" will expire at 2026-02-05 16:56" > > No matter which date format is used, this is harder to understand than > > The password for role "foo" will expire in 1 day(s) > > or > > The password for role "foo" will expire in 22 hour(s) 16 minute(s) > > All 3 are about a password expiring around the same time plus minus a > few hours, but the first one is significantly harder to figure out, > first I have to think about what's the exact date today. I would personally definitely prefer the first one. I have also previously complained about the buildfarm showing relative instead of absolute timestamps, for similar reasons, but I was apparently also in the minority then.
-
Re: Pasword expiration warning
Zsolt Parragi <zsolt.parragi@percona.com> — 2026-02-06T09:53:43Z
> 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. One thing that would help is readability, as macros/constants usually have nice explanatory names. That could be improved by adding a "/* 7 days in seconds */" comment after the number, without a macro. But if somebody searches for the guc name, it's already defined as 7d elsewhere, so I'm not sure if this would be really useful. The patch looks good to me.
-
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-02-06T15:09:49Z
On Fri, Feb 06, 2026 at 09:53:43AM +0000, Zsolt Parragi wrote: > The patch looks good to me. Thanks. I'd like to proceed with committing this patch early next week. If you have any additional feedback or objections, please let me know sooner than later. -- nathan
-
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-02-10T22:48:37Z
Here's what I'm planning to commit tomorrow. -- nathan
-
Re: Pasword expiration warning
Gilles DAROLD <gilles@darold.net> — 2026-02-11T11:32:01Z
Le 10/02/2026 à 23:48, Nathan Bossart a écrit : > Here's what I'm planning to commit tomorrow. Thanks Nathan. -- Gilles Darold
-
Re: Pasword expiration warning
Nathan Bossart <nathandbossart@gmail.com> — 2026-02-11T16:37:12Z
Committed. -- nathan