On Wed, Jan 11, 2023 at 4:11 PM wangw.f...@fujitsu.com <wangw.f...@fujitsu.com> wrote: > > On Mon, Jan 9, 2023 at 13:04 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > Thanks for your comments. > > > One more thing, I think it would be better to expose a new callback > > API via reorder buffer as suggested previously [2] similar to other > > reorder buffer APIs instead of directly using reorderbuffer API to > > invoke plugin API. > > Yes, I agree. I think it would be better to add a new callback API on the > HEAD. > So, I improved the fix approach: > Introduce a new optional callback to update the process. This callback > function > is invoked at the end inside the main loop of the function > ReorderBufferProcessTXN() for each change. In this way, I think it seems that > similar timeout problems could be avoided.
I am a bit worried about the indirections that the wrappers and hooks create. Output plugins call OutputPluginUpdateProgress() in callbacks but I don't see why ReorderBufferProcessTXN() needs a callback to call OutputPluginUpdateProgress. I don't think output plugins are going to do anything special with that callback than just call OutputPluginUpdateProgress. Every output plugin will need to implement it and if they do not they will face the timeout problem. That would be unnecessary. Instead ReorderBufferUpdateProgress() in your first patch was more direct and readable. That way the fix works for any output plugin. In fact, I am wondering whether we could have a call in ReorderBufferProcessTxn() at the end of transaction (commit/prepare/commit prepared/abort prepared) instead of the corresponding output plugin callbacks calling OutputPluginUpdateProgress(). -- Best Wishes, Ashutosh Bapat
