public inbox for [email protected]
help / color / mirror / Atom feedRename setup_cancel_handler in pg_dump
3+ messages / 3 participants
[nested] [flat]
* Rename setup_cancel_handler in pg_dump
@ 2024-01-26 00:42 Yugo NAGATA <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Yugo NAGATA @ 2024-01-26 00:42 UTC (permalink / raw)
To: pgsql-hackers
Hi,
Attached is a simple patch to rename setup_cancel_handler()
in pg_dump/parallel.c.
I am proposing it because there is a public function with
the same name in fe_utils/cancel.c. I know pg_dump/parallel.c
does not include fe_utils/cancel.h, so there is no conflict,
but I think it is better to use different names to reduce
possible confusion.
I guess there was no concerns when setup_cancel_handler in
pg_dump/parallel.c was introduced because the same name
function was not in fe_utils that could be used in common
between client tools.. The public setup_cancel_handler in
fe_utils was introduced in a4fd3aa719e, where this function
was moved from psql.
Regards,
Yugo Nagata
--
Yugo NAGATA <[email protected]>
Attachments:
[text/x-diff] pg_dump_setup_cancel_handler.patch (1.9K, ../../[email protected]/2-pg_dump_setup_cancel_handler.patch)
download | inline diff:
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index 188186829c..261b23cb3f 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -204,7 +204,7 @@ static ParallelSlot *GetMyPSlot(ParallelState *pstate);
static void archive_close_connection(int code, void *arg);
static void ShutdownWorkersHard(ParallelState *pstate);
static void WaitForTerminatingWorkers(ParallelState *pstate);
-static void setup_cancel_handler(void);
+static void pg_dump_setup_cancel_handler(void);
static void set_cancel_pstate(ParallelState *pstate);
static void set_cancel_slot_archive(ParallelSlot *slot, ArchiveHandle *AH);
static void RunWorker(ArchiveHandle *AH, ParallelSlot *slot);
@@ -550,7 +550,7 @@ sigTermHandler(SIGNAL_ARGS)
/*
* Some platforms allow delivery of new signals to interrupt an active
* signal handler. That could muck up our attempt to send PQcancel, so
- * disable the signals that setup_cancel_handler enabled.
+ * disable the signals that pg_dump_setup_cancel_handler enabled.
*/
pqsignal(SIGINT, SIG_IGN);
pqsignal(SIGTERM, SIG_IGN);
@@ -605,7 +605,7 @@ sigTermHandler(SIGNAL_ARGS)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+pg_dump_setup_cancel_handler(void)
{
/*
* When forking, signal_info.handler_set will propagate into the new
@@ -705,7 +705,7 @@ consoleHandler(DWORD dwCtrlType)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+pg_dump_setup_cancel_handler(void)
{
if (!signal_info.handler_set)
{
@@ -737,7 +737,7 @@ set_archive_cancel_info(ArchiveHandle *AH, PGconn *conn)
* important that this happen at least once before we fork off any
* threads.
*/
- setup_cancel_handler();
+ pg_dump_setup_cancel_handler();
/*
* On Unix, we assume that storing a pointer value is atomic with respect
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Rename setup_cancel_handler in pg_dump
@ 2024-01-30 12:44 Daniel Gustafsson <[email protected]>
parent: Yugo NAGATA <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Daniel Gustafsson @ 2024-01-30 12:44 UTC (permalink / raw)
To: Yugo NAGATA <[email protected]>; +Cc: pgsql-hackers
> On 26 Jan 2024, at 01:42, Yugo NAGATA <[email protected]> wrote:
> I am proposing it because there is a public function with
> the same name in fe_utils/cancel.c. I know pg_dump/parallel.c
> does not include fe_utils/cancel.h, so there is no conflict,
> but I think it is better to use different names to reduce
> possible confusion.
Given that a "git grep setup_cancel_hander" returns hits in pg_dump along with
other frontend utils, I can see the risk of confusion.
-setup_cancel_handler(void)
+pg_dump_setup_cancel_handler(void)
We don't have any other functions prefixed with pg_dump_, based on the naming
of the surrounding code in the file I wonder if set_cancel_handler is a more
appropriate name?
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: pgbench --continue-on-error: clarify TPS and failure reporting
@ 2026-07-10 08:36 Xuneng Zhou <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Xuneng Zhou @ 2026-07-10 08:36 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: Fujii Masao <[email protected]>; Yugo Nagata <[email protected]>; pgsql-hackers; Rintaro Ikeda <[email protected]>
Hi Evan,
On Mon, Jun 22, 2026 at 4:11 PM Chao Li <[email protected]> wrote:
>
>
>
> > On Jun 18, 2026, at 20:45, Chao Li <[email protected]> wrote:
> >
> >
> >
> >> On Jun 18, 2026, at 17:40, Fujii Masao <[email protected]> wrote:
> >>
> >> On Thu, Jun 18, 2026 at 3:25 PM Chao Li <[email protected]> wrote:
> >>> Thanks for confirming the doc issue. The here comes a small doc patch.
> >>>
> >>> I also noticed there is a later paragraph about “latency” that also needs an update, it’s included in the patch as well.
> >>
> >> Thanks for the patch! The TPS-related changes look good to me.
> >>
> >> The latency of a successful transaction includes the entire time of
> >> - transaction execution with rollbacks and retries. The latency is measured
> >> - only for successful transactions and commands but not for failed
> >> transactions
> >> - or commands.
> >> + transaction execution with rollbacks and retries. The latency is measured
> >> + only for successful transactions and commands, except that failed
> >> + transactions are included in the simple average latency when the
> >> + <option>--continue-on-error</option> option is used.
> >>
> >> It doesn't seem that the behavior of the latency average in the main
> >> report depends on --continue-on-error itself. Instead, when neither
> >> --progress, --rate, nor --latency-limit is specified, the main
> >> report shows a simple latency average computed as the total benchmark
> >> duration divided by the number of successful and failed transactions,
> >> so failed transactions are included.
> >>
> >> On the other hand, when any of those options is specified, the
> >> latency average and latency stddev in the main report are based
> >> only on successful transactions. Likewise, the per-script and
> >> per-command latency statistics are measured only for successful
> >> transactions and commands.
> >>
> >> So how about something like this instead?
> >>
> >> -------------------------------
> >> <para>
> >> The latency of a successful transaction includes the entire time of
> >> - transaction execution with rollbacks and retries. The simple
> >> - <literal>latency average</literal> shown in the main report is computed from
> >> - the total benchmark duration divided by both successful and failed
> >> - transactions, and therefore includes failed transactions. In contrast,
> >> - detailed latency statistics, including the per-script and per-command
> >> - reports, are measured only for successful transactions and commands, not
> >> - for failed transactions or commands.
> >> + transaction execution with rollbacks and retries. In the main report, when
> >> + neither <option>--progress</option>, <option>--rate</option>, nor
> >> + <option>--latency-limit</option> is specified, the
> >> + <literal>latency average</literal> is computed from the total benchmark
> >> + duration divided by both successful and failed transactions, and therefore
> >> + includes failed transactions. Otherwise, the <literal>latency
> >> average</literal>
> >> + and <literal>latency stddev</literal> shown in the main report are measured
> >> + only for successful transactions. Detailed latency statistics, including
> >> + the per-script and per-command reports, are also measured only for
> >> + successful transactions and commands, not for failed transactions or
> >> + commands.
> >> </para>
> >> -------------------------------
> >>
> >
> > I feel that might be too detailed. Listing these three option names here could make the doc harder to read and maintain. When failures are included, the output line already says so explicitly:
> > ```
> > latency average = 0.387 ms (including failures)
> > ```
> >
> > Can we simply say something like this?
> > ```
> > <para>
> > The latency of a successful transaction includes the entire time of
> > transaction execution with rollbacks and retries. The latency is measured
> > only for successful transactions and commands, not for failed transactions
> > or commands, unless the report explicitly says that it includes failures.
> > </para>
> > ```
> >
> > Best regards,
> > --
> > Chao Li (Evan)
> > HighGo Software Co., Ltd.
> > https://www.highgo.com/
>
> Thinking it over, I came up with a version that combines Fujii-san’s suggestion and my previous version:
> ```
> <para>
> The latency of a successful transaction includes the entire time of
> transaction execution with rollbacks and retries. In the main report,
> when none of <option>--progress</option>, <option>--rate</option>, and
> <option>--latency-limit</option> is specified, <literal>latency average</literal>
> is computed from both successful and failed transactions. In such cases,
> this is explicitly indicated by <literal>(including failures)</literal>.
> Other latency statistics, including the per-script and per-command reports,
> are measured only for successful transactions and commands, not for failed
> transactions or commands.
> </para>
> ```
<para>
This option is useful when your custom script may raise errors
such as unique constraint violations, but you want the benchmark
- to continue and measure performance including those failures.
+ to continue despite individual statement failures. Failed
+ transactions are reported separately, but are not counted as
+ transactions actually processed, nor are they included in TPS.
"Nor are they included in TPS" seems misleading to me. Failed
transactions do not enter the TPS accumulator, but their execution
time remains in bench_duration, so they can reduce the overall TPS.
tps = total->cnt / bench_duration;
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-07-10 08:36 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-01-26 00:42 Rename setup_cancel_handler in pg_dump Yugo NAGATA <[email protected]>
2024-01-30 12:44 ` Daniel Gustafsson <[email protected]>
2026-07-10 08:36 Re: pgbench --continue-on-error: clarify TPS and failure reporting Xuneng Zhou <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox