On Fri, Jan 20, 2023 at 12:35 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > 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.
Agree. Moved the threshold logic to the function ReorderBufferProcessTXN. > > > > ~ > > > > 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. Changed. Invoke the function OutputPluginUpdateProgress directly in the new callback. Regards, Wang Wei
