On Wed, Apr 6, 2022 at 11:09 AM wangw.f...@fujitsu.com <wangw.f...@fujitsu.com> wrote: > > On Fri, Apr 1, 2022 at 12:09 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Apr 1, 2022 at 8:28 AM Euler Taveira <eu...@eulerto.com> wrote: > > > > > > It seems I didn't make myself clear. The callbacks I'm referring to the > > > *_cb_wrapper functions. After every ctx->callbacks.foo_cb() call into a > > > *_cb_wrapper() function, we have something like: > > > > > > if (ctx->progress & PGOUTPUT_PROGRESS_FOO) > > > NewUpdateProgress(ctx, false); > > > > > > The NewUpdateProgress function would contain a logic similar to the > > > update_progress() from the proposed patch. (A different function name here > > just > > > to avoid confusion.) > > > > > > The output plugin is responsible to set ctx->progress with the callback > > > variables (for example, PGOUTPUT_PROGRESS_CHANGE for change_cb()) > > that we would > > > like to run NewUpdateProgress. > > > > > > > This sounds like a conflicting approach to what we currently do. > > Currently, OutputPluginUpdateProgress() is called from the xact > > related pgoutput functions like pgoutput_commit_txn(), > > pgoutput_prepare_txn(), pgoutput_commit_prepared_txn(), etc. So, if we > > follow what you are saying then for some of the APIs like > > pgoutput_change/_message/_truncate, we need to set the parameter to > > invoke NewUpdateProgress() which will internally call > > OutputPluginUpdateProgress(), and for the remaining APIs, we will call > > in the corresponding pgoutput_* function. I feel if we want to make it > > more generic than the current patch, it is better to directly call > > what you are referring to here as NewUpdateProgress() in all remaining > > APIs like pgoutput_change/_truncate, etc. > Thanks for your comments. > > According to your suggestion, improve the patch to make it more generic. > Attach the new patch. >
typedef void (*LogicalOutputPluginWriterUpdateProgress) (struct LogicalDecodingContext *lr, XLogRecPtr Ptr, TransactionId xid, - bool skipped_xact + bool skipped_xact, + bool last_write In this approach, I don't think we need an additional parameter last_write. Let's do the work related to keepalive without a parameter, do you see any problem with that? Also, let's try to evaluate how it impacts lag functionality for large transactions? -- With Regards, Amit Kapila.
