Hi Joel,
Thanks for the patch. After reviewing it, I got a few comments.
On Sep 25, 2025, at 04:34, Joel Jacobson <joel@compiler.org> wrote:
Curious to hear thoughts on this approach.
/Joel
<0001-LISTEN-NOTIFY-make-the-latency-throughput-trade-off-.patch>
1.
```
--- a/src/include/utils/timeout.h
+++ b/src/include/utils/timeout.h
@@ -35,6 +35,7 @@ typedef enum TimeoutId
IDLE_SESSION_TIMEOUT,
IDLE_STATS_UPDATE_TIMEOUT,
CLIENT_CONNECTION_CHECK_TIMEOUT,
+ NOTIFY_DEFERRED_WAKEUP_TIMEOUT,
STARTUP_PROGRESS_TIMEOUT,
```
Can we define the new one after STARTUP_PROGRESS_TIMEOUT to try to preserve the existing enum value?
2.
```
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -766,6 +766,7 @@ autovacuum_worker_slots = 16 # autovacuum worker slots to allocate
#lock_timeout = 0 # in milliseconds, 0 is disabled
#idle_in_transaction_session_timeout = 0 # in milliseconds, 0 is disabled
#idle_session_timeout = 0 # in milliseconds, 0 is disabled
+#notify_latency_target = 0 # in milliseconds, 0 is disabled
#bytea_output = 'hex' # hex, escape
```
I think we should add one more table to make the comment to align with last line’s comment.
3.
```
/* GUC parameters */
bool Trace_notify = false;
+int notify_latency_target;
```
I know compiler will auto initiate notify_latency_target to 0. But all other global and static variables around are explicitly initiated, so it would look better to assign 0 to it, which just keeps coding style consistent.
4.
```
+ /*
+ * Throttling check: if we were last active too recently, defer. This
+ * check is safe without a lock because it's based on a backend-local
+ * timestamp.
+ */
+ if (notify_latency_target > 0 &&
+ !TimestampDifferenceExceeds(last_wakeup_start_time,
+ GetCurrentTimestamp(),
+ notify_latency_target))
+ {
+ /*
+ * Too soon. We leave wakeup_pending_flag untouched (it must be true,
+ * or we wouldn't have been signaled) to tell senders we are
+ * intentionally delaying. Arm a timer to re-awaken and process the
+ * backlog later.
+ */
+ enable_timeout_after(NOTIFY_DEFERRED_WAKEUP_TIMEOUT,
+ notify_latency_target);
+ return;
+ }
+
```
Should we avid duplicate timeout to be enabled? Now, whenever a duplicate notification is avoid, a new timeout is enabled. I think we can add another variable to remember if a timeout has been enabled.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
