Hi Joel, Thanks for the patch. After reviewing it, I got a few comments.
> On Sep 25, 2025, at 04:34, Joel Jacobson <[email protected]> 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/
