On Fri, Jan 20, 2023 at 7:40 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Here are some review comments for patch v3-0001. > > ====== > src/backend/replication/logical/logical.c > > 3. forward declaration > > +/* update progress callback */ > +static void update_progress_cb_wrapper(ReorderBuffer *cache, > + ReorderBufferTXN *txn, > + ReorderBufferChange *change); > > I felt this function wrapper name was a bit misleading... AFAIK every > other wrapper really does just wrap their respective functions. But > this one seems a bit different because it calls the wrapped function > ONLY if some threshold is exceeded. IMO maybe this function could have > some name that conveys this better: > > e.g. update_progress_cb_wrapper_with_threshold >
I am wondering whether it would be better to move the threshold logic to the caller. Previously this logic was inside the function because it was being invoked from multiple places but now that won't be the case. Also, then your concern about the name would also be addressed. > > ~ > > 7b. > Would it be neater to just call OutputPluginUpdateProgress here instead? > > e.g. > BEFORE > ctx->update_progress(ctx, ctx->write_location, ctx->write_xid, false); > AFTER > OutputPluginUpdateProgress(ctx, false); > We already check whether ctx->update_progress is defined or not which is the only extra job done by OutputPluginUpdateProgress but probably we can consolidate the checks and directly invoke OutputPluginUpdateProgress. -- With Regards, Amit Kapila.