Add callbacks for fixed-numbered stats flush in pgstats
Hi all,
The last TODO item I had in my bucket about the generalization of
pgstats is the option to a better control on the flush of the stats
depending on the kind for fixed-numbered stats. Currently, this is
controlled by pgstat_report_stat(), that includes special handling for
WAL, IO and SLRU stats, with two generic concepts:
- Check if there are pending entries, allowing a fast-path exit.
- Do the actual flush, with a recheck on pending entries.
SLRU and IO control that with one variable each, and WAL uses a
routine for the same called pgstat_have_pending_wal(). Please find
attached a patch to generalize the concept, with two new callbacks
that can be used for fixed-numbered stats. SLRU, IO and WAL are
switched to use these (the two pgstat_flush_* routines have been kept
on purpose). This brings some clarity in the code, by making
have_iostats and have_slrustats static in their respective files. The
two pgstat_flush_* wrappers do not need a boolean as return result.
Running Postgres on scissors with a read-only workload that does not
trigger stats, I was not able to see a difference in runtime, but that
was on my own laptop, and I am planning to do more measurements on a
bigger machine.
This is in the same line of thoughts as the recent thread about the
backend init callback, generalizing more the whole facility:
/messages/by-id/ZtZr1K4PLdeWclXY@paquier.xyz
Like the other one, I wanted to send that a few days ago, but well,
life likes going its own ways sometimes.
Thanks,
--
Michael
Attachments:
0001-Add-callbacks-to-control-flush-of-fixed-numbered-sta.patchtext/x-diff; charset=us-asciiDownload
From e98f8d50c17cd8fc521bffb4bdc73ef58b7fa430 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 3 Sep 2024 13:36:10 +0900
Subject: [PATCH] Add callbacks to control flush of fixed-numbered stats
This commit adds two callbacks in pgstats:
- have_fixed_pending_cb, to check if a stats kind has any pending stuff
waiting for a flush.
- flush_fixed_cb, to do the flush.
This is used by the SLRU, WAL and IO statistics, generalizing the
concept for all stats kinds (builtin and custom).
---
src/include/utils/pgstat_internal.h | 42 +++++++++-------
src/backend/utils/activity/pgstat.c | 63 +++++++++++++++++++-----
src/backend/utils/activity/pgstat_io.c | 22 ++++++++-
src/backend/utils/activity/pgstat_slru.c | 13 ++++-
src/backend/utils/activity/pgstat_wal.c | 19 +++++--
5 files changed, 119 insertions(+), 40 deletions(-)
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index fb132e439d..d69aa05b1c 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -231,7 +231,7 @@ typedef struct PgStat_KindInfo
/*
* For variable-numbered stats: flush pending stats. Required if pending
- * data is used.
+ * data is used. See flush_fixed_cb for fixed-numbered stats.
*/
bool (*flush_pending_cb) (PgStat_EntryRef *sr, bool nowait);
@@ -259,6 +259,19 @@ typedef struct PgStat_KindInfo
*/
void (*init_shmem_cb) (void *stats);
+ /*
+ * For fixed-numbered statistics: Flush pending stats. Returns true if
+ * some of the stats could not be flushed, due to lock contention for
+ * example. Optional.
+ */
+ bool (*flush_fixed_cb) (bool nowait);
+
+ /*
+ * For fixed-numbered statistics: Check for pending stats in need of
+ * flush. Returns true if there are any stats pending for flush. Optional.
+ */
+ bool (*have_fixed_pending_cb) (void);
+
/*
* For fixed-numbered statistics: Reset All.
*/
@@ -603,7 +616,10 @@ extern bool pgstat_function_flush_cb(PgStat_EntryRef *entry_ref, bool nowait);
* Functions in pgstat_io.c
*/
-extern bool pgstat_flush_io(bool nowait);
+extern void pgstat_flush_io(bool nowait);
+
+extern bool pgstat_io_have_pending_cb(void);
+extern bool pgstat_io_flush_cb(bool nowait);
extern void pgstat_io_init_shmem_cb(void *stats);
extern void pgstat_io_reset_all_cb(TimestampTz ts);
extern void pgstat_io_snapshot_cb(void);
@@ -662,7 +678,8 @@ extern PgStatShared_Common *pgstat_init_entry(PgStat_Kind kind,
* Functions in pgstat_slru.c
*/
-extern bool pgstat_slru_flush(bool nowait);
+extern bool pgstat_slru_have_pending_cb(void);
+extern bool pgstat_slru_flush_cb(bool nowait);
extern void pgstat_slru_init_shmem_cb(void *stats);
extern void pgstat_slru_reset_all_cb(TimestampTz ts);
extern void pgstat_slru_snapshot_cb(void);
@@ -672,10 +689,11 @@ extern void pgstat_slru_snapshot_cb(void);
* Functions in pgstat_wal.c
*/
-extern bool pgstat_flush_wal(bool nowait);
extern void pgstat_init_wal(void);
-extern bool pgstat_have_pending_wal(void);
+extern void pgstat_flush_wal(bool nowait);
+extern bool pgstat_wal_have_pending_cb(void);
+extern bool pgstat_wal_flush_cb(bool nowait);
extern void pgstat_wal_init_shmem_cb(void *stats);
extern void pgstat_wal_reset_all_cb(TimestampTz ts);
extern void pgstat_wal_snapshot_cb(void);
@@ -705,20 +723,6 @@ extern void pgstat_create_transactional(PgStat_Kind kind, Oid dboid, Oid objoid)
extern PGDLLIMPORT PgStat_LocalState pgStatLocal;
-/*
- * Variables in pgstat_io.c
- */
-
-extern PGDLLIMPORT bool have_iostats;
-
-
-/*
- * Variables in pgstat_slru.c
- */
-
-extern PGDLLIMPORT bool have_slrustats;
-
-
/*
* Implementation of inline functions declared above.
*/
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index b2ca3f39b7..8cbf706d5b 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -411,6 +411,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_off = offsetof(PgStatShared_IO, stats),
.shared_data_len = sizeof(((PgStatShared_IO *) 0)->stats),
+ .flush_fixed_cb = pgstat_io_flush_cb,
+ .have_fixed_pending_cb = pgstat_io_have_pending_cb,
.init_shmem_cb = pgstat_io_init_shmem_cb,
.reset_all_cb = pgstat_io_reset_all_cb,
.snapshot_cb = pgstat_io_snapshot_cb,
@@ -426,6 +428,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_off = offsetof(PgStatShared_SLRU, stats),
.shared_data_len = sizeof(((PgStatShared_SLRU *) 0)->stats),
+ .flush_fixed_cb = pgstat_slru_flush_cb,
+ .have_fixed_pending_cb = pgstat_slru_have_pending_cb,
.init_shmem_cb = pgstat_slru_init_shmem_cb,
.reset_all_cb = pgstat_slru_reset_all_cb,
.snapshot_cb = pgstat_slru_snapshot_cb,
@@ -441,6 +445,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_off = offsetof(PgStatShared_Wal, stats),
.shared_data_len = sizeof(((PgStatShared_Wal *) 0)->stats),
+ .flush_fixed_cb = pgstat_wal_flush_cb,
+ .have_fixed_pending_cb = pgstat_wal_have_pending_cb,
.init_shmem_cb = pgstat_wal_init_shmem_cb,
.reset_all_cb = pgstat_wal_reset_all_cb,
.snapshot_cb = pgstat_wal_snapshot_cb,
@@ -661,13 +667,37 @@ pgstat_report_stat(bool force)
}
/* Don't expend a clock check if nothing to do */
- if (dlist_is_empty(&pgStatPending) &&
- !have_iostats &&
- !have_slrustats &&
- !pgstat_have_pending_wal())
+ if (dlist_is_empty(&pgStatPending))
{
- Assert(pending_since == 0);
- return 0;
+ bool do_flush = false;
+
+ /* Check for pending fixed-numbered stats */
+ for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++)
+ {
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+
+ if (!kind_info)
+ continue;
+ if (!kind_info->fixed_amount)
+ {
+ Assert(kind_info->have_fixed_pending_cb == NULL);
+ continue;
+ }
+ if (!kind_info->have_fixed_pending_cb)
+ continue;
+
+ if (kind_info->have_fixed_pending_cb())
+ {
+ do_flush = true;
+ break;
+ }
+ }
+
+ if (!do_flush)
+ {
+ Assert(pending_since == 0);
+ return 0;
+ }
}
/*
@@ -720,14 +750,23 @@ pgstat_report_stat(bool force)
/* flush database / relation / function / ... stats */
partial_flush |= pgstat_flush_pending_entries(nowait);
- /* flush IO stats */
- partial_flush |= pgstat_flush_io(nowait);
+ /* flush of fixed-numbered stats */
+ for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++)
+ {
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
- /* flush wal stats */
- partial_flush |= pgstat_flush_wal(nowait);
+ if (!kind_info)
+ continue;
+ if (!kind_info->fixed_amount)
+ {
+ Assert(kind_info->flush_fixed_cb == NULL);
+ continue;
+ }
+ if (!kind_info->flush_fixed_cb)
+ continue;
- /* flush SLRU stats */
- partial_flush |= pgstat_slru_flush(nowait);
+ partial_flush |= kind_info->flush_fixed_cb(nowait);
+ }
last_flush = now;
diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c
index 8af55989ee..cc2ffc78aa 100644
--- a/src/backend/utils/activity/pgstat_io.c
+++ b/src/backend/utils/activity/pgstat_io.c
@@ -29,7 +29,7 @@ typedef struct PgStat_PendingIO
static PgStat_PendingIO PendingIOStats;
-bool have_iostats = false;
+static bool have_iostats = false;
/*
@@ -161,6 +161,24 @@ pgstat_fetch_stat_io(void)
return &pgStatLocal.snapshot.io;
}
+/*
+ * Check if there any IO stats waiting for flush.
+ */
+bool
+pgstat_io_have_pending_cb(void)
+{
+ return have_iostats;
+}
+
+/*
+ * Simpler wrapper of pgstat_io_flush_cb()
+ */
+void
+pgstat_flush_io(bool nowait)
+{
+ (void) pgstat_io_flush_cb(nowait);
+}
+
/*
* Flush out locally pending IO statistics
*
@@ -170,7 +188,7 @@ pgstat_fetch_stat_io(void)
* acquired. Otherwise, return false.
*/
bool
-pgstat_flush_io(bool nowait)
+pgstat_io_flush_cb(bool nowait)
{
LWLock *bktype_lock;
PgStat_BktypeIO *bktype_shstats;
diff --git a/src/backend/utils/activity/pgstat_slru.c b/src/backend/utils/activity/pgstat_slru.c
index 6f922a85bf..dd6f9f840e 100644
--- a/src/backend/utils/activity/pgstat_slru.c
+++ b/src/backend/utils/activity/pgstat_slru.c
@@ -32,7 +32,7 @@ static void pgstat_reset_slru_counter_internal(int index, TimestampTz ts);
* in order to avoid memory allocation.
*/
static PgStat_SLRUStats pending_SLRUStats[SLRU_NUM_ELEMENTS];
-bool have_slrustats = false;
+static bool have_slrustats = false;
/*
@@ -143,6 +143,15 @@ pgstat_get_slru_index(const char *name)
return (SLRU_NUM_ELEMENTS - 1);
}
+/*
+ * Check if there are any SLRU stats entries waiting for flush.
+ */
+bool
+pgstat_slru_have_pending_cb(void)
+{
+ return have_slrustats;
+}
+
/*
* Flush out locally pending SLRU stats entries
*
@@ -153,7 +162,7 @@ pgstat_get_slru_index(const char *name)
* acquired. Otherwise return false.
*/
bool
-pgstat_slru_flush(bool nowait)
+pgstat_slru_flush_cb(bool nowait)
{
PgStatShared_SLRU *stats_shmem = &pgStatLocal.shmem->slru;
int i;
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index e2a3f6b865..3d76091f71 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -71,6 +71,15 @@ pgstat_fetch_stat_wal(void)
return &pgStatLocal.snapshot.wal;
}
+/*
+ * Simple wrapper of pgstat_wal_flush_cb()
+ */
+void
+pgstat_flush_wal(bool nowait)
+{
+ (void) pgstat_wal_flush_cb(nowait);
+}
+
/*
* Calculate how much WAL usage counters have increased by subtracting the
* previous counters from the current ones.
@@ -79,7 +88,7 @@ pgstat_fetch_stat_wal(void)
* acquired. Otherwise return false.
*/
bool
-pgstat_flush_wal(bool nowait)
+pgstat_wal_flush_cb(bool nowait)
{
PgStatShared_Wal *stats_shmem = &pgStatLocal.shmem->wal;
WalUsage wal_usage_diff = {0};
@@ -92,7 +101,7 @@ pgstat_flush_wal(bool nowait)
* This function can be called even if nothing at all has happened. Avoid
* taking lock for nothing in that case.
*/
- if (!pgstat_have_pending_wal())
+ if (!pgstat_wal_have_pending_cb())
return false;
/*
@@ -141,8 +150,8 @@ void
pgstat_init_wal(void)
{
/*
- * Initialize prevWalUsage with pgWalUsage so that pgstat_flush_wal() can
- * calculate how much pgWalUsage counters are increased by subtracting
+ * Initialize prevWalUsage with pgWalUsage so that pgstat_wal_flush_cb()
+ * can calculate how much pgWalUsage counters are increased by subtracting
* prevWalUsage from pgWalUsage.
*/
prevWalUsage = pgWalUsage;
@@ -156,7 +165,7 @@ pgstat_init_wal(void)
* data pages.
*/
bool
-pgstat_have_pending_wal(void)
+pgstat_wal_have_pending_cb(void)
{
return pgWalUsage.wal_records != prevWalUsage.wal_records ||
PendingWalStats.wal_write != 0 ||
--
2.45.2
At Tue, 3 Sep 2024 13:48:59 +0900, Michael Paquier <michael@paquier.xyz> wrote in
Hi all,
The last TODO item I had in my bucket about the generalization of
pgstats is the option to a better control on the flush of the stats
depending on the kind for fixed-numbered stats. Currently, this is
controlled by pgstat_report_stat(), that includes special handling for
WAL, IO and SLRU stats, with two generic concepts:
- Check if there are pending entries, allowing a fast-path exit.
- Do the actual flush, with a recheck on pending entries.SLRU and IO control that with one variable each, and WAL uses a
routine for the same called pgstat_have_pending_wal(). Please find
attached a patch to generalize the concept, with two new callbacks
that can be used for fixed-numbered stats. SLRU, IO and WAL are
switched to use these (the two pgstat_flush_* routines have been kept
on purpose). This brings some clarity in the code, by making
have_iostats and have_slrustats static in their respective files. The
two pgstat_flush_* wrappers do not need a boolean as return result.
The generalization sounds good to me, and hiding the private flags in
private places also seems good.
Regarding pgstat_io_flush_cb, I think it no longer needs to check the
have_iostats variable, and that check should be moved to the new
pgstat_flush_io(). The same applies to pgstat_wal_flush_cb() (and
pgstat_flush_wal()). As for pgstat_slru_flush_cb, it simply doesn't
need the check anymore.
Running Postgres on scissors with a read-only workload that does not
trigger stats, I was not able to see a difference in runtime, but that
was on my own laptop, and I am planning to do more measurements on a
bigger machine.
I don't think it matters, since the actual flushing occurs at
10-second intervals during busy times. We could change the check from
a callback to a variable, but that would just shift the function call
overhead to a more frequently called side.
This is in the same line of thoughts as the recent thread about the
backend init callback, generalizing more the whole facility:
/messages/by-id/ZtZr1K4PLdeWclXY@paquier.xyzLike the other one, I wanted to send that a few days ago, but well,
life likes going its own ways sometimes.
reards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Hi,
On Wed, Sep 04, 2024 at 02:05:46PM +0900, Kyotaro Horiguchi wrote:
At Tue, 3 Sep 2024 13:48:59 +0900, Michael Paquier <michael@paquier.xyz> wrote in
Hi all,
The last TODO item I had in my bucket about the generalization of
pgstats is the option to a better control on the flush of the stats
depending on the kind for fixed-numbered stats. Currently, this is
controlled by pgstat_report_stat(), that includes special handling for
WAL, IO and SLRU stats, with two generic concepts:
- Check if there are pending entries, allowing a fast-path exit.
- Do the actual flush, with a recheck on pending entries.SLRU and IO control that with one variable each, and WAL uses a
routine for the same called pgstat_have_pending_wal(). Please find
attached a patch to generalize the concept, with two new callbacks
that can be used for fixed-numbered stats. SLRU, IO and WAL are
switched to use these (the two pgstat_flush_* routines have been kept
on purpose). This brings some clarity in the code, by making
have_iostats and have_slrustats static in their respective files. The
two pgstat_flush_* wrappers do not need a boolean as return result.The generalization sounds good to me, and hiding the private flags in
private places also seems good.
+1 on the idea.
Regarding pgstat_io_flush_cb, I think it no longer needs to check the
have_iostats variable, and that check should be moved to the new
pgstat_flush_io(). The same applies to pgstat_wal_flush_cb() (and
pgstat_flush_wal()). As for pgstat_slru_flush_cb, it simply doesn't
need the check anymore.
Agree. Another option could be to add only one callback (the flush_fixed_cb one)
and get rid of the have_fixed_pending_cb one. Then add an extra parameter to the
flush_fixed_cb that would only check if there is pending stats (without
flushing them). I think those 2 callbacks are highly related that's why I
think we could "merge" them, thoughts?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Wed, Sep 04, 2024 at 05:28:56AM +0000, Bertrand Drouvot wrote:
On Wed, Sep 04, 2024 at 02:05:46PM +0900, Kyotaro Horiguchi wrote:
The generalization sounds good to me, and hiding the private flags in
private places also seems good.+1 on the idea.
Thanks for the feedback.
Regarding pgstat_io_flush_cb, I think it no longer needs to check the
have_iostats variable, and that check should be moved to the new
pgstat_flush_io(). The same applies to pgstat_wal_flush_cb() (and
pgstat_flush_wal()). As for pgstat_slru_flush_cb, it simply doesn't
need the check anymore.Agree. Another option could be to add only one callback (the flush_fixed_cb one)
and get rid of the have_fixed_pending_cb one. Then add an extra parameter to the
flush_fixed_cb that would only check if there is pending stats (without
flushing them). I think those 2 callbacks are highly related that's why I
think we could "merge" them, thoughts?
I would still value the shortcut that we can use if there is no
activity to avoid the clock check with GetCurrentTimestamp(), though,
which is why I'd rather stick with two callbacks as that can lead to a
much cheaper path.
Anyway, I am not sure to follow your point about removing the boolean
checks in the flush callbacks. It's surely always a better to never
call LWLockAcquire() or LWLockConditionalAcquire() if we don't have
to, and we would go through the whole flush dance as long as we detect
some stats activity in at least one stats kind. I mean, that's cheap
enough to keep around.
--
Michael
Hi,
On Wed, Sep 04, 2024 at 03:12:37PM +0900, Michael Paquier wrote:
On Wed, Sep 04, 2024 at 05:28:56AM +0000, Bertrand Drouvot wrote:
On Wed, Sep 04, 2024 at 02:05:46PM +0900, Kyotaro Horiguchi wrote:
The generalization sounds good to me, and hiding the private flags in
private places also seems good.+1 on the idea.
Thanks for the feedback.
Regarding pgstat_io_flush_cb, I think it no longer needs to check the
have_iostats variable, and that check should be moved to the new
pgstat_flush_io(). The same applies to pgstat_wal_flush_cb() (and
pgstat_flush_wal()). As for pgstat_slru_flush_cb, it simply doesn't
need the check anymore.Agree. Another option could be to add only one callback (the flush_fixed_cb one)
and get rid of the have_fixed_pending_cb one. Then add an extra parameter to the
flush_fixed_cb that would only check if there is pending stats (without
flushing them). I think those 2 callbacks are highly related that's why I
think we could "merge" them, thoughts?I would still value the shortcut that we can use if there is no
activity to avoid the clock check with GetCurrentTimestamp(),
Agree. The idea was to add an additional parameter (say "check_only") to the
flush_fixed_cb. If this parameter is set to true then the flush_fixed_cb would
do nothing (no flush at all) but return a boolean that would indicate if there
is pending stats. In case it returns false, then we could avoid the clock check.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
At Wed, 4 Sep 2024 15:12:37 +0900, Michael Paquier <michael@paquier.xyz> wrote in
On Wed, Sep 04, 2024 at 05:28:56AM +0000, Bertrand Drouvot wrote:
On Wed, Sep 04, 2024 at 02:05:46PM +0900, Kyotaro Horiguchi wrote:
The generalization sounds good to me, and hiding the private flags in
private places also seems good.+1 on the idea.
Thanks for the feedback.
Regarding pgstat_io_flush_cb, I think it no longer needs to check the
have_iostats variable, and that check should be moved to the new
pgstat_flush_io(). The same applies to pgstat_wal_flush_cb() (and
pgstat_flush_wal()). As for pgstat_slru_flush_cb, it simply doesn't
need the check anymore.Agree. Another option could be to add only one callback (the flush_fixed_cb one)
and get rid of the have_fixed_pending_cb one. Then add an extra parameter to the
flush_fixed_cb that would only check if there is pending stats (without
flushing them). I think those 2 callbacks are highly related that's why I
think we could "merge" them, thoughts?I would still value the shortcut that we can use if there is no
activity to avoid the clock check with GetCurrentTimestamp(), though,
which is why I'd rather stick with two callbacks as that can lead to a
much cheaper path.
Yeah, I was wrong in this point.
Anyway, I am not sure to follow your point about removing the boolean
checks in the flush callbacks. It's surely always a better to never
call LWLockAcquire() or LWLockConditionalAcquire() if we don't have
to, and we would go through the whole flush dance as long as we detect
some stats activity in at least one stats kind. I mean, that's cheap
enough to keep around.
I doubt that overprotection is always better, but in this case, it's
not overprotection at all. The flush callbacks are called
unconditionally once we decide to flush anything. Sorry for the noise.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Wed, Sep 04, 2024 at 06:37:01PM +0900, Kyotaro Horiguchi wrote:
I doubt that overprotection is always better, but in this case, it's
not overprotection at all. The flush callbacks are called
unconditionally once we decide to flush anything. Sorry for the noise.
Yes, it's intended to be a fastpath, with checks happening if we have
no data pending for any variable-numbered stats.
--
Michael
On Wed, Sep 04, 2024 at 06:27:43AM +0000, Bertrand Drouvot wrote:
Agree. The idea was to add an additional parameter (say "check_only") to the
flush_fixed_cb. If this parameter is set to true then the flush_fixed_cb would
do nothing (no flush at all) but return a boolean that would indicate if there
is pending stats. In case it returns false, then we could avoid the clock check.
Hmm. It's the same thing in terms of logic, but I am not really
convinced that it is a good idea to mix a code path for a sanity check
with a code path dealing with the action, particularly considering
that there is the nowait option to think about in the flush callback
which would require one to think about 4 possible states rather than
two possibilities. So that's more error-prone for extension
developers, IMO.
At the end, I have used my original suggestions for the callbacks. If
there are voices in favor of something different, we still have a good
chunk of the development and beta cycle for that.
--
Michael