From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: cleanup patches for incremental backup
Date: Thu, 14 Mar 2024 16:00:10 -0500
Message-ID: <20240314210010.GA3056455@nathanxps13> (raw)
In-Reply-To: <20240124180515.GA2709722@nathanxps13>
References: <CA+TgmoZd=pYk0owL4W2f4to9qFjwq_NmGJzXg44pEcMxuYriCA@mail.gmail.com>
<CAEze2WizrxyWg+xeVMuyVqfZc-8U1myy1V4H3zwV-7xXztVf2w@mail.gmail.com>
<CA+TgmoadPUdt8ZJ_Zem-T2_qZ5OTXVdxH6wGtnaV-TunN+LOHQ@mail.gmail.com>
<CAEze2Wjbf==0=v+Ck8B2vcCeqD_3_SfDV3HtpEQiP-r3e2fsSA@mail.gmail.com>
<CA+TgmobTfzrnBesboXutZmrABdP79mO2HhXupY5YOdZ9VLWFog@mail.gmail.com>
<CAEze2WjhdVCqEe_qqEok3NA6DwUdOGSBjAxzmYdAqiaaH1uRcg@mail.gmail.com>
<CA+TgmoaQHtGNFczJzSO5sMobUpuikSHphtFPZpdLmjzwdmM1DQ@mail.gmail.com>
<20240124170846.GA2643050@nathanxps13>
<CA+TgmoZusu5g3rM1k=UB29Sf53c1OKjm1uFw9uo-sSXBLFZJiQ@mail.gmail.com>
<20240124180515.GA2709722@nathanxps13>
On Wed, Jan 24, 2024 at 12:05:15PM -0600, Nathan Bossart wrote:
> There might be an overflow risk in the cutoff time calculation, but I doubt
> that's the root cause of these failures:
>
> /*
> * Files should only be removed if the last modification time precedes the
> * cutoff time we compute here.
> */
> cutoff_time = time(NULL) - 60 * wal_summary_keep_time;
I've attached a short patch for fixing this overflow risk. Specifically,
it limits wal_summary_keep_time to INT_MAX / SECS_PER_MINUTE, just like
log_rotation_age.
I considering checking for overflow when we subtract the keep-time from the
result of time(2), but AFAICT there's only a problem if time_t is unsigned,
which Wikipedia leads me to believe is unusual [0], so I figured we might
be able to just wait this one out until 2038.
> Otherwise, I think we'll probably need to add some additional logging to
> figure out what is happening...
Separately, I suppose it's probably time to revert the temporary debugging
code adding by commit 5ddf997. I can craft a patch for that, too.
[0] https://en.wikipedia.org/wiki/Unix_time#Representing_the_number
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.comAttachments:overflow_fix.patch (text/x-diff, 1.5K) downloaddiff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.cindex ec2874c18c..c820d1f9ed 100644--- a/src/backend/postmaster/walsummarizer.c+++ b/src/backend/postmaster/walsummarizer.c@@ -139,7 +139,7 @@ static XLogRecPtr redo_pointer_at_last_summary_removal = InvalidXLogRecPtr;
* GUC parameters
*/
bool summarize_wal = false;
-int wal_summary_keep_time = 10 * 24 * 60;+int wal_summary_keep_time = 10 * HOURS_PER_DAY * MINS_PER_HOUR;
static void WalSummarizerShutdown(int code, Datum arg);
static XLogRecPtr GetLatestLSN(TimeLineID *tli);
@@ -1474,7 +1474,7 @@ MaybeRemoveOldWalSummaries(void)
* Files should only be removed if the last modification time precedes the
* cutoff time we compute here.
*/
- cutoff_time = time(NULL) - 60 * wal_summary_keep_time;+ cutoff_time = time(NULL) - wal_summary_keep_time * SECS_PER_MINUTE;
/* Get all the summaries that currently exist. */
wslist = GetWalSummaries(0, InvalidXLogRecPtr, InvalidXLogRecPtr);
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.cindex 57d9de4dd9..1e71e7db4a 100644--- a/src/backend/utils/misc/guc_tables.c+++ b/src/backend/utils/misc/guc_tables.c@@ -3293,9 +3293,9 @@ struct config_int ConfigureNamesInt[] =
GUC_UNIT_MIN,
},
&wal_summary_keep_time,
- 10 * 24 * 60, /* 10 days */+ 10 * HOURS_PER_DAY * MINS_PER_HOUR, /* 10 days */
0,
- INT_MAX,+ INT_MAX / SECS_PER_MINUTE,
NULL, NULL, NULL
},
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, robertmhaas@gmail.com, boekewurm+postgres@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: cleanup patches for incremental backup
In-Reply-To: <20240314210010.GA3056455@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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