Dear Wang,
Thank you for making a patch.
I applied your patch and confirmed that codes passed regression test.
I put a short reviewing:
```
+ static int skipped_changes_count = 0;
+ /*
+ * Conservatively, at least 150,000 changes can be skipped in 1s.
+ *
+ * Because we use half of wal_sender_timeout as the threshold, and the
unit
+ * of wal_sender_timeout in process is ms, the final threshold is
+ * wal_sender_timeout * 75.
+ */
+ int skipped_changes_threshold = wal_sender_timeout * 75;
```
I'm not sure but could you tell me the background of this calculation?
Is this assumption reasonable?
```
@@ -654,20 +663,62 @@ pgoutput_change(LogicalDecodingContext *ctx,
ReorderBufferTXN *txn,
{
case REORDER_BUFFER_CHANGE_INSERT:
if (!relentry->pubactions.pubinsert)
+ {
+ if (++skipped_changes_count >=
skipped_changes_threshold)
+ {
+ OutputPluginUpdateProgress(ctx, true);
+
+ /*
+ * After sending keepalive message,
reset
+ * skipped_changes_count.
+ */
+ skipped_changes_count = 0;
+ }
return;
+ }
break;
```
Is the if-statement needed? In the walsender case OutputPluginUpdateProgress()
leads WalSndUpdateProgress(),
and the function also has the threshold for ping-ing.
```
static void
-WalSndUpdateProgress(LogicalDecodingContext *ctx, XLogRecPtr lsn,
TransactionId xid)
+WalSndUpdateProgress(LogicalDecodingContext *ctx, XLogRecPtr lsn,
TransactionId xid, bool send_keep_alive)
{
- static TimestampTz sendTime = 0;
+ static TimestampTz trackTime = 0;
TimestampTz now = GetCurrentTimestamp();
+ if (send_keep_alive)
+ {
+ /*
+ * If half of wal_sender_timeout has lapsed without send
message standby,
+ * send a keep-alive message to the standby.
+ */
+ static TimestampTz sendTime = 0;
+ TimestampTz ping_time = TimestampTzPlusMilliseconds(sendTime,
+
wal_sender_timeout / 2);
+ if (now >= ping_time)
+ {
+ WalSndKeepalive(false);
+
+ /* Try to flush pending output to the client */
+ if (pq_flush_if_writable() != 0)
+ WalSndShutdown();
+ sendTime = now;
+ }
+ }
+
```
* +1 about renaming to trackTime.
* `/2` might be magic number. How about following? Renaming is very welcome:
```
+#define WALSND_LOGICAL_PING_FACTOR 0.5
+ static TimestampTz sendTime = 0;
+ TimestampTz ping_time = TimestampTzPlusMilliseconds(sendTime,
+
wal_sender_timeout * WALSND_LOGICAL_PING_FACTOR)
```
Could you add a commitfest entry for cfbot?
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
