On Tues, Jan 24, 2023 at 8:28 AM Peter Smith <smithpb2...@gmail.com> wrote: > Hi Hou-san, Here are my review comments for v5-0001.
Thanks for your comments. > ====== > src/backend/replication/logical/reorderbuffer.c > > 1. > @@ -2446,6 +2452,23 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, > ReorderBufferTXN *txn, > elog(ERROR, "tuplecid value in changequeue"); > break; > } > + > + /* > + * Sending keepalive messages after every change has some overhead, but > + * testing showed there is no noticeable overhead if keepalive is only > + * sent after every ~100 changes. > + */ > +#define CHANGES_THRESHOLD 100 > + > + /* > + * Try to send a keepalive message after every CHANGES_THRESHOLD > + * changes. > + */ > + if (++changes_count >= CHANGES_THRESHOLD) > + { > + rb->update_progress_txn(rb, txn, change); > + changes_count = 0; > + } > > I noticed you put the #define adjacent to the only usage of it, > instead of with the other variable declaration like it was before. > Probably it is better how you have done it, but: > > 1a. > The comment indentation is incorrect. > > ~ > > 1b. > Since the #define is adjacent to its only usage IMO now the 2nd > comment is redundant. So the code can just say > > /* > * Sending keepalive messages after every change has some > overhead, but > * testing showed there is no noticeable overhead if > keepalive is only > * sent after every ~100 changes. > */ > #define CHANGES_THRESHOLD 100 > if (++changes_count >= CHANGES_THRESHOLD) > { > rb->update_progress_txn(rb, txn, change); > changes_count = 0; > } Changed as suggested. Attach the new patch. Regards, Wang Wei
v6-0001-Fix-the-logical-replication-timeout-during-proces.patch
Description: v6-0001-Fix-the-logical-replication-timeout-during-proces.patch