pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedpg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
14+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2024-11-22 efdc7d7475 Add INT64_HEX_FORMAT and UINT64_HEX_FORMAT to c.h.
2024-10-07 c91d0af0a2 b255493aef a356d23fd3 8aaf88b63d 01731eeeaa 5d6187d2a2 5cea7168df Fix Y2038 issues with MyStartTime.
* pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-09-24 19:33 Max Johnson <max.johnson@novatechautomation.com>
0 siblings, 1 reply; 14+ messages in thread
From: Max Johnson @ 2024-09-24 19:33 UTC (permalink / raw)
To: pgsql-hackers
Hello hackers,
I have found an instance of a time overflow with the start time that is written in "postmaster.pid". On a 32-bit Linux system, if the system date is past 01/19/2038, when you start Postgres with `pg_ctl start -D {datadir} ...`, the start time will have rolled back to approximately 1900. This is an instance of the "2038 problem". On my system, pg_ctl will not exit if the start time has overflowed.
This can be fixed by casting "MyStartTime" to a long long instead of just a long in "src/backend/utils/init/miscinit.c". Additionally, in "src/bin/pg_ctl/pg_ctl.c", when we read that value from the file, we should use "atoll()" instead of "atol()" to ensure we are reading it as a long long.
I have verified that this fixes the start time overflow on my 32-bit arm system. My glibc is compiled with 64-bit time_t.
Most systems running Postgres likely aren't 32-bit, but for embedded systems, this is important to ensure 2038 compatibility.
This is a fairly trivial patch, and I do not currently see any issues with using long long. I was told on IRC that a regression test is likely not necessary for this patch.
I look forward to hearing any feedback. This is my first open-source contribution!
Thank you,
Max Johnson
Embedded Linux Engineer I
NovaTech, LLC
13555 W. 107th Street | Lenexa, KS 66215
O: 913.451.1880
M: 913.742.4580
novatechautomation.com<http://www.novatechautomation.com/; | NovaTechLinkedIn<https://www.linkedin.com/company/565017;
NovaTech Automation is Net Zero committed. #KeepItCool<https://www.keepitcool.earth/;
Receipt of this email implies compliance with our terms and conditions<https://www.novatechautomation.com/email-terms-conditions;.
Attachments:
0001-pg_ctl-and-miscinit-change-type-of-MyStartTime.patch (text/x-patch, 1.9K) download
From 70ec929f971577e3a78ab32d15b631954286aaf7 Mon Sep 17 00:00:00 2001
From: Max Johnson <max.johnson@novatechautomation.com>
Date: Mon, 23 Sep 2024 13:41:43 -0500
Subject: [PATCH] pg_ctl and miscinit: change type of MyStartTime
The start time variable needs to be 64 bits wide on all platforms in order
to support dates past 01/19/2038. This patch fixes an overflow error on
the start time in "postmaster.pid", which can cause pg_ctl to hang.
Change the value of "MyStartTime" to long long.
Signed-off-by: Max Johnson <max.johnson@novatechautomation.com>
---
src/backend/utils/init/miscinit.c | 4 ++--
src/bin/pg_ctl/pg_ctl.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 1f0b3175ae..8669e95817 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1205,10 +1205,10 @@ CreateLockFile(const char *filename, bool amPostmaster,
* both datadir and socket lockfiles; although more stuff may get added to
* the datadir lockfile later.
*/
- snprintf(buffer, sizeof(buffer), "%d\n%s\n%ld\n%d\n%s\n",
+ snprintf(buffer, sizeof(buffer), "%d\n%s\n%lld\n%d\n%s\n",
amPostmaster ? (int) my_pid : -((int) my_pid),
DataDir,
- (long) MyStartTime,
+ (long long) MyStartTime,
PostPortNumber,
socketDir);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index f9e0ee4eee..e138c5b236 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -624,7 +624,7 @@ wait_for_postmaster_start(pgpid_t pm_pid, bool do_checkpoint)
* Allow 2 seconds slop for possible cross-process clock skew.
*/
pmpid = atol(optlines[LOCK_FILE_LINE_PID - 1]);
- pmstart = atol(optlines[LOCK_FILE_LINE_START_TIME - 1]);
+ pmstart = atoll(optlines[LOCK_FILE_LINE_START_TIME - 1]);
if (pmstart >= start_time - 2 &&
#ifndef WIN32
pmpid == pm_pid
--
2.34.1
* Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-09-24 20:26 Nathan Bossart <nathandbossart@gmail.com>
parent: Max Johnson <max.johnson@novatechautomation.com>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2024-09-24 20:26 UTC (permalink / raw)
To: Max Johnson <max.johnson@novatechautomation.com>; +Cc: pgsql-hackers
On Tue, Sep 24, 2024 at 07:33:24PM +0000, Max Johnson wrote:
> I have found an instance of a time overflow with the start time that is
> written in "postmaster.pid". On a 32-bit Linux system, if the system date
> is past 01/19/2038, when you start Postgres with `pg_ctl start -D
> {datadir} ...`, the start time will have rolled back to approximately
> 1900. This is an instance of the "2038 problem". On my system, pg_ctl
> will not exit if the start time has overflowed.
Nice find. I think this has been the case since the start time was added
to the lock files [0].
> - snprintf(buffer, sizeof(buffer), "%d\n%s\n%ld\n%d\n%s\n",
> + snprintf(buffer, sizeof(buffer), "%d\n%s\n%lld\n%d\n%s\n",
> amPostmaster ? (int) my_pid : -((int) my_pid),
> DataDir,
> - (long) MyStartTime,
> + (long long) MyStartTime,
> PostPortNumber,
> socketDir);
I think we should use INT64_FORMAT here. That'll choose the right length
modifier for the platform. And I don't think we need to cast MyStartTime,
since it's a pg_time_t (which is just an int64).
[0] https://postgr.es/c/30aeda4
--
nathan
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-09-24 20:44 Tom Lane <tgl@sss.pgh.pa.us>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Tom Lane @ 2024-09-24 20:44 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Max Johnson <max.johnson@novatechautomation.com>; pgsql-hackers
Nathan Bossart <nathandbossart@gmail.com> writes:
> I think we should use INT64_FORMAT here. That'll choose the right length
> modifier for the platform. And I don't think we need to cast MyStartTime,
> since it's a pg_time_t (which is just an int64).
Agreed. However, a quick grep finds half a dozen other places that
are casting MyStartTime to long. We should fix them all.
Also note that if any of the other places are using translatable
format strings, INT64_FORMAT is problematic in that context, and
"long long" is a better answer for them.
(I've not dug in the history, but I rather imagine that this is all
a hangover from MyStartTime having once been plain "time_t".)
regards, tom lane
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-09-24 20:58 Nathan Bossart <nathandbossart@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2024-09-24 20:58 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Max Johnson <max.johnson@novatechautomation.com>; pgsql-hackers
On Tue, Sep 24, 2024 at 04:44:41PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> I think we should use INT64_FORMAT here. That'll choose the right length
>> modifier for the platform. And I don't think we need to cast MyStartTime,
>> since it's a pg_time_t (which is just an int64).
>
> Agreed. However, a quick grep finds half a dozen other places that
> are casting MyStartTime to long. We should fix them all.
+1
> Also note that if any of the other places are using translatable
> format strings, INT64_FORMAT is problematic in that context, and
> "long long" is a better answer for them.
At a glance, I'm not seeing any translatable format strings that involve
MyStartTime. But that is good to know...
--
nathan
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-09-25 15:17 Max Johnson <max.johnson@novatechautomation.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Max Johnson @ 2024-09-25 15:17 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: tgl@sss.pgh.pa.us <tgl@sss.pgh.pa.us>; pgsql-hackers
Hi there,
I have amended my patch to reflect the changes that were discussed and have verified on my system that it works the same as before. I have also fixed a typo and changed the name of the patch to more accurately reflect what it does now. Please let me know if there is anything else you'd like me to do.
Thanks again,
Max Johnson
Embedded Linux Engineer I
NovaTech, LLC
13555 W. 107th Street | Lenexa, KS 66215
O: 913.451.1880
M: 913.742.4580
novatechautomation.com<http://www.novatechautomation.com/; | NovaTechLinkedIn<https://www.linkedin.com/company/565017;
NovaTech Automation is Net Zero committed. #KeepItCool<https://www.keepitcool.earth/;
Receipt of this email implies compliance with our terms and conditions<https://www.novatechautomation.com/email-terms-conditions;.
________________________________
From: Nathan Bossart <nathandbossart@gmail.com>
Sent: Tuesday, September 24, 2024 3:58 PM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Max Johnson <max.johnson@novatechautomation.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
On Tue, Sep 24, 2024 at 04:44:41PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> I think we should use INT64_FORMAT here. That'll choose the right length
>> modifier for the platform. And I don't think we need to cast MyStartTime,
>> since it's a pg_time_t (which is just an int64).
>
> Agreed. However, a quick grep finds half a dozen other places that
> are casting MyStartTime to long. We should fix them all.
+1
> Also note that if any of the other places are using translatable
> format strings, INT64_FORMAT is problematic in that context, and
> "long long" is a better answer for them.
At a glance, I'm not seeing any translatable format strings that involve
MyStartTime. But that is good to know...
--
nathan
Attachments:
0001-pg_ctl-miscinit-don-t-cast-MyStartTime-to-long.patch (text/x-patch, 1.9K) download
From 031944d8045f13df474d307608a2246306b06f2e Mon Sep 17 00:00:00 2001
From: Max Johnson <max.johnson@novatechautomation.com>
Date: Wed, 25 Sep 2024 10:05:46 -0500
Subject: [PATCH] pg_ctl/miscinit: don't cast MyStartTime to long.
The start time variable needs to be 64 bits wide on all platforms in
order to support dates past 01/19/2038. This patch fixes an overflow
error on 32-bit systems with the start time in "postmaster.pid", which
causes pg_ctl to hang.
Don't cast MyStartTime to long.
Signed-off-by: Max Johnson <max.johnson@novatechautomation.com>
---
src/backend/utils/init/miscinit.c | 4 ++--
src/bin/pg_ctl/pg_ctl.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 1f0b3175ae..33f8f49fa2 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1205,10 +1205,10 @@ CreateLockFile(const char *filename, bool amPostmaster,
* both datadir and socket lockfiles; although more stuff may get added to
* the datadir lockfile later.
*/
- snprintf(buffer, sizeof(buffer), "%d\n%s\n%ld\n%d\n%s\n",
+ snprintf(buffer, sizeof(buffer), "%d\n%s\n"INT64_FORMAT"\n%d\n%s\n",
amPostmaster ? (int) my_pid : -((int) my_pid),
DataDir,
- (long) MyStartTime,
+ MyStartTime,
PostPortNumber,
socketDir);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index f9e0ee4eee..e138c5b236 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -624,7 +624,7 @@ wait_for_postmaster_start(pgpid_t pm_pid, bool do_checkpoint)
* Allow 2 seconds slop for possible cross-process clock skew.
*/
pmpid = atol(optlines[LOCK_FILE_LINE_PID - 1]);
- pmstart = atol(optlines[LOCK_FILE_LINE_START_TIME - 1]);
+ pmstart = atoll(optlines[LOCK_FILE_LINE_START_TIME - 1]);
if (pmstart >= start_time - 2 &&
#ifndef WIN32
pmpid == pm_pid
--
2.34.1
* Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-09-25 18:48 Nathan Bossart <nathandbossart@gmail.com>
parent: Max Johnson <max.johnson@novatechautomation.com>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2024-09-25 18:48 UTC (permalink / raw)
To: Max Johnson <max.johnson@novatechautomation.com>; +Cc: tgl@sss.pgh.pa.us <tgl@sss.pgh.pa.us>; pgsql-hackers
On Wed, Sep 25, 2024 at 03:17:45PM +0000, Max Johnson wrote:
> I have amended my patch to reflect the changes that were discussed and
> have verified on my system that it works the same as before. I have also
> fixed a typo and changed the name of the patch to more accurately reflect
> what it does now. Please let me know if there is anything else you'd like
> me to do.
Thanks! I went through all the other uses of MyStartTime and fixed those
as needed, too. Please let me know what you think.
--
nathan
Attachments:
fix_mystarttime_y2k38_bugs.patch (text/plain, 4.5K) download
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 630b304338..da16fc78d4 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -522,7 +522,8 @@ process_pgfdw_appname(const char *appname)
appendStringInfoString(&buf, application_name);
break;
case 'c':
- appendStringInfo(&buf, "%lx.%x", (long) (MyStartTime), MyProcPid);
+ appendStringInfo(&buf, INT64_HEX_FORMAT ".%x",
+ MyStartTime, MyProcPid);
break;
case 'C':
appendStringInfoString(&buf, cluster_name);
diff --git a/src/backend/utils/error/csvlog.c b/src/backend/utils/error/csvlog.c
index 855e130a97..acdffb6d06 100644
--- a/src/backend/utils/error/csvlog.c
+++ b/src/backend/utils/error/csvlog.c
@@ -120,7 +120,7 @@ write_csvlog(ErrorData *edata)
appendStringInfoChar(&buf, ',');
/* session id */
- appendStringInfo(&buf, "%lx.%x", (long) MyStartTime, MyProcPid);
+ appendStringInfo(&buf, INT64_HEX_FORMAT ".%x", MyStartTime, MyProcPid);
appendStringInfoChar(&buf, ',');
/* Line number */
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5cbb5b5416..70e4c30df3 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2944,12 +2944,13 @@ log_status_format(StringInfo buf, const char *format, ErrorData *edata)
{
char strfbuf[128];
- snprintf(strfbuf, sizeof(strfbuf) - 1, "%lx.%x",
- (long) (MyStartTime), MyProcPid);
+ snprintf(strfbuf, sizeof(strfbuf) - 1, INT64_HEX_FORMAT ".%x",
+ MyStartTime, MyProcPid);
appendStringInfo(buf, "%*s", padding, strfbuf);
}
else
- appendStringInfo(buf, "%lx.%x", (long) (MyStartTime), MyProcPid);
+ appendStringInfo(buf, INT64_HEX_FORMAT ".%x",
+ MyStartTime, MyProcPid);
break;
case 'p':
if (padding != 0)
diff --git a/src/backend/utils/error/jsonlog.c b/src/backend/utils/error/jsonlog.c
index bd0124869d..492383a89e 100644
--- a/src/backend/utils/error/jsonlog.c
+++ b/src/backend/utils/error/jsonlog.c
@@ -168,8 +168,8 @@ write_jsonlog(ErrorData *edata)
}
/* Session id */
- appendJSONKeyValueFmt(&buf, "session_id", true, "%lx.%x",
- (long) MyStartTime, MyProcPid);
+ appendJSONKeyValueFmt(&buf, "session_id", true, INT64_HEX_FORMAT ".%x",
+ MyStartTime, MyProcPid);
/* Line number */
appendJSONKeyValueFmt(&buf, "line_num", false, "%ld", log_line_number);
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 537d92c0cf..ef60f41b8c 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1372,10 +1372,10 @@ CreateLockFile(const char *filename, bool amPostmaster,
* both datadir and socket lockfiles; although more stuff may get added to
* the datadir lockfile later.
*/
- snprintf(buffer, sizeof(buffer), "%d\n%s\n%ld\n%d\n%s\n",
+ snprintf(buffer, sizeof(buffer), "%d\n%s\n" INT64_FORMAT "\n%d\n%s\n",
amPostmaster ? (int) my_pid : -((int) my_pid),
DataDir,
- (long) MyStartTime,
+ MyStartTime,
PostPortNumber,
socketDir);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index e7e878c22f..d6bb2c3311 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -618,7 +618,7 @@ wait_for_postmaster_start(pid_t pm_pid, bool do_checkpoint)
* Allow 2 seconds slop for possible cross-process clock skew.
*/
pmpid = atol(optlines[LOCK_FILE_LINE_PID - 1]);
- pmstart = atol(optlines[LOCK_FILE_LINE_START_TIME - 1]);
+ pmstart = atoll(optlines[LOCK_FILE_LINE_START_TIME - 1]);
if (pmstart >= start_time - 2 &&
#ifndef WIN32
pmpid == pm_pid
diff --git a/src/include/c.h b/src/include/c.h
index dc1841346c..400c527397 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -547,6 +547,8 @@ typedef unsigned long long int uint64;
/* snprintf format strings to use for 64-bit integers */
#define INT64_FORMAT "%" INT64_MODIFIER "d"
#define UINT64_FORMAT "%" INT64_MODIFIER "u"
+#define INT64_HEX_FORMAT "%" INT64_MODIFIER "x"
+#define UINT64_HEX_FORMAT "%" INT64_MODIFIER "x"
/*
* 128-bit signed and unsigned integers
diff --git a/src/test/modules/test_radixtree/test_radixtree.c b/src/test/modules/test_radixtree/test_radixtree.c
index 1d9165a3a2..3e6863f660 100644
--- a/src/test/modules/test_radixtree/test_radixtree.c
+++ b/src/test/modules/test_radixtree/test_radixtree.c
@@ -23,8 +23,6 @@
/* uncomment to use shared memory for the tree */
/* #define TEST_SHARED_RT */
-#define UINT64_HEX_FORMAT "%" INT64_MODIFIER "X"
-
/* Convenient macros to test results */
#define EXPECT_TRUE(expr) \
do { \
* Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-09-25 20:04 Max Johnson <max.johnson@novatechautomation.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Max Johnson @ 2024-09-25 20:04 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers; tgl@sss.pgh.pa.us <tgl@sss.pgh.pa.us>
Hi Nathan,
I think your patch looks good, no objections. I am happy to have contributed.
Thanks,
Max
________________________________
From: Nathan Bossart <nathandbossart@gmail.com>
Sent: Wednesday, September 25, 2024 1:48 PM
To: Max Johnson <max.johnson@novatechautomation.com>
Cc: tgl@sss.pgh.pa.us <tgl@sss.pgh.pa.us>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
On Wed, Sep 25, 2024 at 03:17:45PM +0000, Max Johnson wrote:
> I have amended my patch to reflect the changes that were discussed and
> have verified on my system that it works the same as before. I have also
> fixed a typo and changed the name of the patch to more accurately reflect
> what it does now. Please let me know if there is anything else you'd like
> me to do.
Thanks! I went through all the other uses of MyStartTime and fixed those
as needed, too. Please let me know what you think.
--
nathan
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-09-27 02:38 Nathan Bossart <nathandbossart@gmail.com>
parent: Max Johnson <max.johnson@novatechautomation.com>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2024-09-27 02:38 UTC (permalink / raw)
To: Max Johnson <max.johnson@novatechautomation.com>; +Cc: pgsql-hackers; tgl@sss.pgh.pa.us <tgl@sss.pgh.pa.us>
On Wed, Sep 25, 2024 at 08:04:59PM +0000, Max Johnson wrote:
> I think your patch looks good, no objections. I am happy to have contributed.
Great. I've attached what I have staged for commit.
My first instinct was to not bother back-patching this since all
currently-supported versions will have been out of support for over 8 years
by the time this becomes a practical issue. However, I wonder if it makes
sense to back-patch for the kinds of 32-bit embedded systems you cited
upthread. I can imagine that such systems might need to work for a very
long time without any software updates, in which case it'd probably be a
good idea to make this fix available in the next minor release. What do
you think?
--
nathan
Attachments:
v4-0001-Fix-Y2K38-issues-with-MyStartTime.patch (text/plain, 6.2K) download
From 84f05c822f44b8b8ec437540bbae6bb43c72e916 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 26 Sep 2024 21:00:41 -0500
Subject: [PATCH v4 1/1] Fix Y2K38 issues with MyStartTime.
Several places treat MyStartTime as a "long", which is only 32 bits
wide on some platforms. In reality, MyStartTime is a pg_time_t,
i.e., a signed 64-bit integer. This will lead to interesting bugs
on the aforementioned systems in 2038 when signed 32-bit integers
are no longer sufficient to store Unix time (e.g., "pg_ctl start"
hanging). To fix, ensure that MyStartTime is handled as a 64-bit
value everywhere. (Of course, users will need to ensure that
time_t is 64 bits wide on their system, too, but there's little
that Postgres can do about that.)
This could probably be back-patched, but since all
currently-supported versions will have been out of support for over
8 years by the time this becomes a practical issue, I'm not going
to bother.
Co-authored-by: Max Johnson
Discussion: https://postgr.es/m/CO1PR07MB905262E8AC270FAAACED66008D682%40CO1PR07MB9052.namprd07.prod.outlook.com
---
contrib/postgres_fdw/option.c | 3 ++-
src/backend/utils/error/csvlog.c | 2 +-
src/backend/utils/error/elog.c | 7 ++++---
src/backend/utils/error/jsonlog.c | 4 ++--
src/backend/utils/init/miscinit.c | 4 ++--
src/bin/pg_ctl/pg_ctl.c | 2 +-
src/include/c.h | 2 ++
src/test/modules/test_radixtree/test_radixtree.c | 2 --
8 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 630b304338..da16fc78d4 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -522,7 +522,8 @@ process_pgfdw_appname(const char *appname)
appendStringInfoString(&buf, application_name);
break;
case 'c':
- appendStringInfo(&buf, "%lx.%x", (long) (MyStartTime), MyProcPid);
+ appendStringInfo(&buf, INT64_HEX_FORMAT ".%x",
+ MyStartTime, MyProcPid);
break;
case 'C':
appendStringInfoString(&buf, cluster_name);
diff --git a/src/backend/utils/error/csvlog.c b/src/backend/utils/error/csvlog.c
index 855e130a97..acdffb6d06 100644
--- a/src/backend/utils/error/csvlog.c
+++ b/src/backend/utils/error/csvlog.c
@@ -120,7 +120,7 @@ write_csvlog(ErrorData *edata)
appendStringInfoChar(&buf, ',');
/* session id */
- appendStringInfo(&buf, "%lx.%x", (long) MyStartTime, MyProcPid);
+ appendStringInfo(&buf, INT64_HEX_FORMAT ".%x", MyStartTime, MyProcPid);
appendStringInfoChar(&buf, ',');
/* Line number */
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5cbb5b5416..70e4c30df3 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2944,12 +2944,13 @@ log_status_format(StringInfo buf, const char *format, ErrorData *edata)
{
char strfbuf[128];
- snprintf(strfbuf, sizeof(strfbuf) - 1, "%lx.%x",
- (long) (MyStartTime), MyProcPid);
+ snprintf(strfbuf, sizeof(strfbuf) - 1, INT64_HEX_FORMAT ".%x",
+ MyStartTime, MyProcPid);
appendStringInfo(buf, "%*s", padding, strfbuf);
}
else
- appendStringInfo(buf, "%lx.%x", (long) (MyStartTime), MyProcPid);
+ appendStringInfo(buf, INT64_HEX_FORMAT ".%x",
+ MyStartTime, MyProcPid);
break;
case 'p':
if (padding != 0)
diff --git a/src/backend/utils/error/jsonlog.c b/src/backend/utils/error/jsonlog.c
index bd0124869d..492383a89e 100644
--- a/src/backend/utils/error/jsonlog.c
+++ b/src/backend/utils/error/jsonlog.c
@@ -168,8 +168,8 @@ write_jsonlog(ErrorData *edata)
}
/* Session id */
- appendJSONKeyValueFmt(&buf, "session_id", true, "%lx.%x",
- (long) MyStartTime, MyProcPid);
+ appendJSONKeyValueFmt(&buf, "session_id", true, INT64_HEX_FORMAT ".%x",
+ MyStartTime, MyProcPid);
/* Line number */
appendJSONKeyValueFmt(&buf, "line_num", false, "%ld", log_line_number);
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 537d92c0cf..ef60f41b8c 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1372,10 +1372,10 @@ CreateLockFile(const char *filename, bool amPostmaster,
* both datadir and socket lockfiles; although more stuff may get added to
* the datadir lockfile later.
*/
- snprintf(buffer, sizeof(buffer), "%d\n%s\n%ld\n%d\n%s\n",
+ snprintf(buffer, sizeof(buffer), "%d\n%s\n" INT64_FORMAT "\n%d\n%s\n",
amPostmaster ? (int) my_pid : -((int) my_pid),
DataDir,
- (long) MyStartTime,
+ MyStartTime,
PostPortNumber,
socketDir);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index e7e878c22f..d6bb2c3311 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -618,7 +618,7 @@ wait_for_postmaster_start(pid_t pm_pid, bool do_checkpoint)
* Allow 2 seconds slop for possible cross-process clock skew.
*/
pmpid = atol(optlines[LOCK_FILE_LINE_PID - 1]);
- pmstart = atol(optlines[LOCK_FILE_LINE_START_TIME - 1]);
+ pmstart = atoll(optlines[LOCK_FILE_LINE_START_TIME - 1]);
if (pmstart >= start_time - 2 &&
#ifndef WIN32
pmpid == pm_pid
diff --git a/src/include/c.h b/src/include/c.h
index dc1841346c..400c527397 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -547,6 +547,8 @@ typedef unsigned long long int uint64;
/* snprintf format strings to use for 64-bit integers */
#define INT64_FORMAT "%" INT64_MODIFIER "d"
#define UINT64_FORMAT "%" INT64_MODIFIER "u"
+#define INT64_HEX_FORMAT "%" INT64_MODIFIER "x"
+#define UINT64_HEX_FORMAT "%" INT64_MODIFIER "x"
/*
* 128-bit signed and unsigned integers
diff --git a/src/test/modules/test_radixtree/test_radixtree.c b/src/test/modules/test_radixtree/test_radixtree.c
index 1d9165a3a2..3e6863f660 100644
--- a/src/test/modules/test_radixtree/test_radixtree.c
+++ b/src/test/modules/test_radixtree/test_radixtree.c
@@ -23,8 +23,6 @@
/* uncomment to use shared memory for the tree */
/* #define TEST_SHARED_RT */
-#define UINT64_HEX_FORMAT "%" INT64_MODIFIER "X"
-
/* Convenient macros to test results */
#define EXPECT_TRUE(expr) \
do { \
--
2.39.5 (Apple Git-154)
* Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-09-27 14:48 Max Johnson <max.johnson@novatechautomation.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Max Johnson @ 2024-09-27 14:48 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
I think that it would be a good idea to include these fixes in the next minor release. After working for a couple months on getting our embedded systems 2038 compliant, it has become very apparent that 2038 will be a substantial ordeal. Maximizing the number of systems that include this fix would make things a little easier when that time comes around.
Thanks,
Max
________________________________
From: Nathan Bossart <nathandbossart@gmail.com>
Sent: Thursday, September 26, 2024 9:38 PM
To: Max Johnson <max.johnson@novatechautomation.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>; tgl@sss.pgh.pa.us <tgl@sss.pgh.pa.us>
Subject: Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
On Wed, Sep 25, 2024 at 08:04:59PM +0000, Max Johnson wrote:
> I think your patch looks good, no objections. I am happy to have contributed.
Great. I've attached what I have staged for commit.
My first instinct was to not bother back-patching this since all
currently-supported versions will have been out of support for over 8 years
by the time this becomes a practical issue. However, I wonder if it makes
sense to back-patch for the kinds of 32-bit embedded systems you cited
upthread. I can imagine that such systems might need to work for a very
long time without any software updates, in which case it'd probably be a
good idea to make this fix available in the next minor release. What do
you think?
--
nathan
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-09-27 19:10 Nathan Bossart <nathandbossart@gmail.com>
parent: Max Johnson <max.johnson@novatechautomation.com>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2024-09-27 19:10 UTC (permalink / raw)
To: Max Johnson <max.johnson@novatechautomation.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Fri, Sep 27, 2024 at 02:48:01PM +0000, Max Johnson wrote:
> I think that it would be a good idea to include these fixes in the next
> minor release. After working for a couple months on getting our embedded
> systems 2038 compliant, it has become very apparent that 2038 will be a
> substantial ordeal. Maximizing the number of systems that include this
> fix would make things a little easier when that time comes around.
Alright. I was able to back-patch it to v12 without too much trouble,
fortunately. I'll commit that soon unless anyone else has feedback.
--
nathan
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-10-07 19:00 Nathan Bossart <nathandbossart@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2024-10-07 19:00 UTC (permalink / raw)
To: Max Johnson <max.johnson@novatechautomation.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Fri, Sep 27, 2024 at 02:10:47PM -0500, Nathan Bossart wrote:
> Alright. I was able to back-patch it to v12 without too much trouble,
> fortunately. I'll commit that soon unless anyone else has feedback.
Committed, thanks!
I refrained from introducing INT64_HEX_FORMAT/UINT64_HEX_FORMAT in c.h
because I felt there was a nonzero chance of that causing problems with
third-party code on the back-branches. We could probably add them for v18,
though.
--
nathan
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-10-07 19:17 Nathan Bossart <nathandbossart@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2024-10-07 19:17 UTC (permalink / raw)
To: Max Johnson <max.johnson@novatechautomation.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Mon, Oct 07, 2024 at 02:00:05PM -0500, Nathan Bossart wrote:
> I refrained from introducing INT64_HEX_FORMAT/UINT64_HEX_FORMAT in c.h
> because I felt there was a nonzero chance of that causing problems with
> third-party code on the back-branches. We could probably add them for v18,
> though.
Like so...
--
nathan
Attachments:
0001-add-INT64_HEX_FORMAT-and-UINT64_HEX_FORMAT.patch (text/plain, 3.8K) download
From 03f7536acd06b91e6891ccfc86470a5a51b45736 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 7 Oct 2024 14:16:07 -0500
Subject: [PATCH 1/1] add INT64_HEX_FORMAT and UINT64_HEX_FORMAT
---
contrib/postgres_fdw/option.c | 2 +-
src/backend/utils/error/csvlog.c | 2 +-
src/backend/utils/error/elog.c | 4 ++--
src/backend/utils/error/jsonlog.c | 2 +-
src/include/c.h | 2 ++
src/test/modules/test_radixtree/test_radixtree.c | 2 --
6 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index d740893918..9a6785720d 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -522,7 +522,7 @@ process_pgfdw_appname(const char *appname)
appendStringInfoString(&buf, application_name);
break;
case 'c':
- appendStringInfo(&buf, "%" INT64_MODIFIER "x.%x", MyStartTime, MyProcPid);
+ appendStringInfo(&buf, INT64_HEX_FORMAT ".%x", MyStartTime, MyProcPid);
break;
case 'C':
appendStringInfoString(&buf, cluster_name);
diff --git a/src/backend/utils/error/csvlog.c b/src/backend/utils/error/csvlog.c
index eab8df3fcc..acdffb6d06 100644
--- a/src/backend/utils/error/csvlog.c
+++ b/src/backend/utils/error/csvlog.c
@@ -120,7 +120,7 @@ write_csvlog(ErrorData *edata)
appendStringInfoChar(&buf, ',');
/* session id */
- appendStringInfo(&buf, "%" INT64_MODIFIER "x.%x", MyStartTime, MyProcPid);
+ appendStringInfo(&buf, INT64_HEX_FORMAT ".%x", MyStartTime, MyProcPid);
appendStringInfoChar(&buf, ',');
/* Line number */
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 987ff98067..c71508c923 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2944,12 +2944,12 @@ log_status_format(StringInfo buf, const char *format, ErrorData *edata)
{
char strfbuf[128];
- snprintf(strfbuf, sizeof(strfbuf) - 1, "%" INT64_MODIFIER "x.%x",
+ snprintf(strfbuf, sizeof(strfbuf) - 1, INT64_HEX_FORMAT ".%x",
MyStartTime, MyProcPid);
appendStringInfo(buf, "%*s", padding, strfbuf);
}
else
- appendStringInfo(buf, "%" INT64_MODIFIER "x.%x", MyStartTime, MyProcPid);
+ appendStringInfo(buf, INT64_HEX_FORMAT ".%x", MyStartTime, MyProcPid);
break;
case 'p':
if (padding != 0)
diff --git a/src/backend/utils/error/jsonlog.c b/src/backend/utils/error/jsonlog.c
index 2c7b14cacb..492383a89e 100644
--- a/src/backend/utils/error/jsonlog.c
+++ b/src/backend/utils/error/jsonlog.c
@@ -168,7 +168,7 @@ write_jsonlog(ErrorData *edata)
}
/* Session id */
- appendJSONKeyValueFmt(&buf, "session_id", true, "%" INT64_MODIFIER "x.%x",
+ appendJSONKeyValueFmt(&buf, "session_id", true, INT64_HEX_FORMAT ".%x",
MyStartTime, MyProcPid);
/* Line number */
diff --git a/src/include/c.h b/src/include/c.h
index 3297d89ff0..9520b89b00 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -550,6 +550,8 @@ typedef unsigned long long int uint64;
/* snprintf format strings to use for 64-bit integers */
#define INT64_FORMAT "%" INT64_MODIFIER "d"
#define UINT64_FORMAT "%" INT64_MODIFIER "u"
+#define INT64_HEX_FORMAT "%" INT64_MODIFIER "x"
+#define UINT64_HEX_FORMAT "%" INT64_MODIFIER "x"
/*
* 128-bit signed and unsigned integers
diff --git a/src/test/modules/test_radixtree/test_radixtree.c b/src/test/modules/test_radixtree/test_radixtree.c
index 1d9165a3a2..3e6863f660 100644
--- a/src/test/modules/test_radixtree/test_radixtree.c
+++ b/src/test/modules/test_radixtree/test_radixtree.c
@@ -23,8 +23,6 @@
/* uncomment to use shared memory for the tree */
/* #define TEST_SHARED_RT */
-#define UINT64_HEX_FORMAT "%" INT64_MODIFIER "X"
-
/* Convenient macros to test results */
#define EXPECT_TRUE(expr) \
do { \
--
2.39.5 (Apple Git-154)
* Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-11-19 23:00 Nathan Bossart <nathandbossart@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2024-11-19 23:00 UTC (permalink / raw)
To: Max Johnson <max.johnson@novatechautomation.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Mon, Oct 07, 2024 at 02:17:06PM -0500, Nathan Bossart wrote:
> On Mon, Oct 07, 2024 at 02:00:05PM -0500, Nathan Bossart wrote:
>> I refrained from introducing INT64_HEX_FORMAT/UINT64_HEX_FORMAT in c.h
>> because I felt there was a nonzero chance of that causing problems with
>> third-party code on the back-branches. We could probably add them for v18,
>> though.
>
> Like so...
If there are no concerns, I plan on committing this soon.
--
nathan
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem.
@ 2024-11-22 18:43 Nathan Bossart <nathandbossart@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 0 replies; 14+ messages in thread
From: Nathan Bossart @ 2024-11-22 18:43 UTC (permalink / raw)
To: Max Johnson <max.johnson@novatechautomation.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Tue, Nov 19, 2024 at 05:00:43PM -0600, Nathan Bossart wrote:
> If there are no concerns, I plan on committing this soon.
Committed.
--
nathan
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2024-11-22 18:43 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-09-24 19:33 pg_ctl/miscinit: print "MyStartTime" as a long long instead of long to avoid 2038 problem. Max Johnson <max.johnson@novatechautomation.com>
2024-09-24 20:26 ` Nathan Bossart <nathandbossart@gmail.com>
2024-09-24 20:44 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-09-24 20:58 ` Nathan Bossart <nathandbossart@gmail.com>
2024-09-25 15:17 ` Max Johnson <max.johnson@novatechautomation.com>
2024-09-25 18:48 ` Nathan Bossart <nathandbossart@gmail.com>
2024-09-25 20:04 ` Max Johnson <max.johnson@novatechautomation.com>
2024-09-27 02:38 ` Nathan Bossart <nathandbossart@gmail.com>
2024-09-27 14:48 ` Max Johnson <max.johnson@novatechautomation.com>
2024-09-27 19:10 ` Nathan Bossart <nathandbossart@gmail.com>
2024-10-07 19:00 ` Nathan Bossart <nathandbossart@gmail.com>
2024-10-07 19:17 ` Nathan Bossart <nathandbossart@gmail.com>
2024-11-19 23:00 ` Nathan Bossart <nathandbossart@gmail.com>
2024-11-22 18:43 ` Nathan Bossart <nathandbossart@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox