Subject: Re: Logical replication timeout problem

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 16 Jan 2023 22:06:43 +0530

On Wed, Jan 11, 2023 at 4:11 PM [email-masked]
<[email-masked]> wrote:
>
> On Mon, Jan 9, 2023 at 13:04 PM Amit Kapila <[email-masked]> 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

---

From: Amit Kapila <[email-masked]>
Date: Tue, 17 Jan 2023 15:34:02 +0530

On Mon, Jan 16, 2023 at 10:06 PM Ashutosh Bapat
<[email-masked]> wrote:
> call OutputPluginUpdateProgress.
>

Yeah, I think we can do it as we are doing the previous approach but
we need an additional wrapper (update_progress_cb_wrapper()) as the
current patch has so that we can add error context information. This
is similar to why we have a wrapper for all other callbacks like
change_cb_wrapper.

-- 
With Regards,
Amit Kapila.

---

From: Ashutosh Bapat <[email-masked]>
Date: Tue, 17 Jan 2023 18:41:46 +0530

> is similar to why we have a wrapper for all other callbacks like

Ultimately OutputPluginUpdateProgress() will be called - which in turn
will call ctx->update_progress. I don't see wrappers around
OutputPluginWrite or OutputPluginPrepareWrite. But I see that those
two are called always from output plugin, so indirectly those are
called through a wrapper. I also see that update_progress_cb_wrapper()
is similar, as far as wrapper is concerned, to
ReorderBufferUpdateProgress() in the earlier patch.
ReorderBufferUpdateProgress() looks more readable than the wrapper.

If we want to keep the wrapper at least we should use a different
variable name. update_progress is also there LogicalDecodingContext
and will be indirectly called from ReorderBuffer::update_progress.
Somebody might think that there's some recursion involved there.
That's a mighty confusion.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Amit Kapila <[email-masked]>
Date: Wed, 18 Jan 2023 10:58:34 +0530

On Tue, Jan 17, 2023 at 6:41 PM Ashutosh Bapat
<[email-masked]> wrote:
> will call ctx->update_progress.
>

No, update_progress_cb_wrapper() should directly call
ctx->update_progress(). The key reason to have a
update_progress_cb_wrapper() is that it allows us to add error context
information (see the usage of output_plugin_error_callback).

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 18 Jan 2023 08:19:08 +0000

> information (see the usage of output_plugin_error_callback).

I think it makes sense. This also avoids the need for every output plugin to
implement the callback. So I tried to improve the patch based on this approach.

And I tried to add some comments for this new callback to distinguish it from
ctx->update_progress.

Attach the new patch.

Regards,
Wang Wei

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 18 Jan 2023 17:37:19 +0530

> And I tried to add some comments for this new callback to distinguish it from

Comments don't help when using cscope or some such code browsing tool.
Better to use a different variable name.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Amit Kapila <[email-masked]>
Date: Wed, 18 Jan 2023 18:00:12 +0530

> Better to use a different variable name.

+ /*
+ * Callback to be called when updating progress during sending data of a
+ * transaction (and its subtransactions) to the output plugin.
+ */
+ ReorderBufferUpdateProgressCB update_progress;

Are you suggesting changing the name of the above variable? If so, how
about apply_progress, progress, or updateprogress? If you don't like
any of these then feel free to suggest something else. If we change
the variable name then accordingly, we need to update
ReorderBufferUpdateProgressCB as well.

-- 
With Regards,
Amit Kapila.

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 19 Jan 2023 16:13:23 +0530

> ReorderBufferUpdateProgressCB as well.

I would liked to have all the callback names renamed with prefix
"rbcb_xxx" so that they have very less chances of conflicting with
similar names in the code base. But it's probably late to do that :).

How are update_txn_progress since the CB is supposed to be used only
within a transaction? or update_progress_txn?
update_progress_cb_wrapper needs a change of name as well.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Amit Kapila <[email-masked]>
Date: Thu, 19 Jan 2023 17:07:12 +0530

> within a transaction? or update_progress_txn?

Personally, I would prefer 'apply_progress' as it would be similar to
a few other callbacks like apply_change, apply_truncate, or as is
proposed by patch update_progress again because it is similar to
existing callbacks like commit_prepared. If you and others don't like
any of those then we can go for 'update_progress_txn' as well. Anybody
else has an opinion on this?

-- 
With Regards,
Amit Kapila.