From: Sami Imseih <samimseih@gmail.com>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: Restart pg_usleep when interrupted
Date: Wed, 7 Aug 2024 16:22:22 -0500
Message-ID: <D1558DDA-CEED-4CED-B3ED-C425D996FCA3@gmail.com> (raw)
In-Reply-To: <ZrOGi8RZTSHxTq9W@nathan>
References: <ZpVaXQGEspp3gk8S@nathan>
<ZpX/5/irF/gZqT7b@ip-10-97-1-34.eu-west-3.compute.internal>
<CDF34F36-3A52-47C8-B1D3-5D32A34EC3A7@gmail.com>
<ZqNd7WfTm3sUCpjy@ip-10-97-1-34.eu-west-3.compute.internal>
<33287B8A-3CA0-494F-87D3-C9D85C6468FE@gmail.com>
<ZqiIxptiTDqRTxDx@ip-10-97-1-34.eu-west-3.compute.internal>
<19E276C9-2C2B-435A-B275-8FA22222AEB8@gmail.com>
<ZrHmE3zUSNXzoHCU@ip-10-97-1-34.eu-west-3.compute.internal>
<50E1D6AD-3E51-4E45-95A7-7B20DC037DCB@gmail.com>
<ZrMNlUd1h09nMrNQ@ip-10-97-1-34.eu-west-3.compute.internal>
<ZrOGi8RZTSHxTq9W@nathan>
>
> On Wed, Aug 07, 2024 at 06:00:53AM +0000, Bertrand Drouvot wrote:
>> + SleepEx((microsec < 500 ? 1 : (microsec + 500) / 1000), FALSE);
>
> I think this deserves a comment.
>
Done
>> +#define INSTR_TIME_ADD_MICROSEC(x,t) \
>> + ((x).ticks += t * NS_PER_US)
>
> I'd add parentheses around "t" to ensure any expressions given as "t" are
> evaluated first.
>
Done
> Also, do we need to worry about overflow here? It looks like the rest of
> instr_time.h is oblivious about overflow, so maybe this is better discussed
> in a separate thread...
>
I agree, this needs to be handled in a different thread.
Please see v7.
Regards,
Sami
Attachments:v7-0001-vaccum_delay-with-absolute-time-nanosleep.patch (application/octet-stream, 3.4K) download
From 83ef3f489e39f031d70f779582cdc276d0d415d1 Mon Sep 17 00:00:00 2001
From: Sami Imseih <simseih@amazon.com>
Date: Wed, 7 Aug 2024 10:09:00 -0500
Subject: [PATCH v7 1/1] vaccum_delay with absolute time nanosleep
---
src/backend/commands/vacuum.c | 57 +++++++++++++++++++++++++++-
src/include/portability/instr_time.h | 10 +++++
2 files changed, 66 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.cindex 48f8eab202..4edffd3bc5 100644--- a/src/backend/commands/vacuum.c+++ b/src/backend/commands/vacuum.c@@ -116,6 +116,61 @@ static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
static double compute_parallel_delay(void);
static VacOptValue get_vacoptval_from_boolean(DefElem *def);
static bool vac_tid_reaped(ItemPointer itemptr, void *state);
+static void vacuum_sleep(double msec);++/*+ * Delay vacuum for a specified time.+ *+ * Unlike pg_usleep, This function continues the delay in case of an+ * interrupt.+ */+static void+vacuum_sleep(double msec)+{+ long microsec = msec * 1000;++ if (microsec > 0)+ {+#ifndef WIN32+ /*+ * We allow nanosleep to handle interrupts and retry with the+ * remaining time. However, frequent interruptions and restarts of the+ * nanosleep calls can substantially lead to drift in the time when+ * the sleep finally completes. To deal with this, we break out of the+ * loop whenever the current time is past the expected end time of the+ * sleep.+ */+ struct timespec delay;+ struct timespec remain;+ instr_time end_time;++ INSTR_TIME_SET_CURRENT(end_time);+ INSTR_TIME_ADD_MICROSEC(end_time, microsec);++ delay.tv_sec = microsec / 1000000L;+ delay.tv_nsec = (microsec % 1000000L) * 1000;++ while (nanosleep(&delay, &remain) == -1 && errno == EINTR)+ {+ instr_time current_time;++ INSTR_TIME_SET_CURRENT(current_time);++ if (INSTR_TIME_IS_GREATER(current_time, end_time))+ break;++ delay = remain;+ }+#else+ /*+ * Win32 uses SleepEx to perform the delay. It is non-interruptable,+ * therefore no special handling is required to restart the sleep with+ * remaining time.+ */+ SleepEx((microsec < 500 ? 1 : (microsec + 500) / 1000), FALSE);+#endif+ }+}
/*
* GUC check function to ensure GUC value specified is within the allowable
@@ -2384,7 +2439,7 @@ vacuum_delay_point(void)
msec = vacuum_cost_delay * 4;
pgstat_report_wait_start(WAIT_EVENT_VACUUM_DELAY);
- pg_usleep(msec * 1000);+ vacuum_sleep(msec);
pgstat_report_wait_end();
/*
diff --git a/src/include/portability/instr_time.h b/src/include/portability/instr_time.hindex e66ecf34cd..6e4b0f1b17 100644--- a/src/include/portability/instr_time.h+++ b/src/include/portability/instr_time.h@@ -36,6 +36,10 @@
*
* INSTR_TIME_GET_NANOSEC(t) convert t to int64 (in nanoseconds)
*
+ * INSTR_TIME_ADD_MICROSEC(x,t) add t (in microseconds) to x+ *+ * INSTR_TIME_IS_GREATER(x,y) is x greater than y?+ *
* Note that INSTR_TIME_SUBTRACT and INSTR_TIME_ACCUM_DIFF convert
* absolute times to intervals. The INSTR_TIME_GET_xxx operations are
* only useful on intervals.
@@ -194,4 +198,10 @@ GetTimerFrequency(void)
#define INSTR_TIME_GET_MICROSEC(t) \
(INSTR_TIME_GET_NANOSEC(t) / NS_PER_US)
+#define INSTR_TIME_ADD_MICROSEC(x,t) \+ ((x).ticks += (t) * NS_PER_US)++#define INSTR_TIME_IS_GREATER(x,y) \+ ((x).ticks > (y).ticks)+
#endif /* INSTR_TIME_H */
--
2.39.3 (Apple Git-146)
=
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: samimseih@gmail.com, nathandbossart@gmail.com, bertranddrouvot.pg@gmail.com, tgl@sss.pgh.pa.us, robertmhaas@gmail.com
Subject: Re: Restart pg_usleep when interrupted
In-Reply-To: <D1558DDA-CEED-4CED-B3ED-C425D996FCA3@gmail.com>
* 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