RE: Rework LogicalOutputPluginWriterUpdateProgress

On Tue, Mar 7, 2023 15:55 PM Kuroda, Hayato/黒田 隼人 <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> Dear Wang,
>
> Thank you for updating the patch! Followings are my comments.

Thanks for your comments.

> ---
> 01. missing comments
> You might miss the comment from Peter[1]. Or could you pin the related one?

Since I think the functions WalSndPrepareWrite and WalSndWriteData have similar
parameters and the HEAD has no related comments, I'm not sure whether we should
add them in this patch, or in a separate patch to comment atop these callback
functions or where they are called.

> ---
> 02. LogicalDecodingProcessRecord()
>
> Don't we have to call UpdateDecodingProgressAndKeepalive() when there is no
> decoding function? Assuming that the timeout parameter does not have enough
> time
> period and there are so many sequential operations in the transaction. At that
> time
> there may be a possibility that timeout is occurred while calling
> ReorderBufferProcessXid()
> several times. It may be a bad example, but I meant to say that we may have to
> consider the case that decoding function has not implemented yet.

I think it's ok in this function. If the decoding function has not been
implemented for a record, I think we quickly return to the loop in the function
WalSndLoop, where it will try to send the keepalive message.

BTW, in the previous discussion [1], we decided to ignore some paths, because
the gain from modifying them may not be so great.

> ---
> 03. stream_*_cb_wrapper
>
> Only stream_*_cb_wrapper have comments "don't call update progress, we
> didn't really make any", but
> there are more functions that does not send updates. Do you have any reasons
> why only they have?

Added this comment to more functions.
I think the following six functions don't call the function
UpdateProgressAndKeepalive in v5 patch:
- begin_cb_wrapper
- begin_prepare_cb_wrapper
- startup_cb_wrapper
- shutdown_cb_wrapper
- filter_prepare_cb_wrapper
- filter_by_origin_cb_wrapper

I think the comment you mentioned means that no new progress needs to be updated
in this *_cb_wrapper. Also, I think we don't need to update the progress at the
beginning of a transaction, just like in HEAD. So, I added the same comment only
in the 4 functions below:
- startup_cb_wrapper
- shutdown_cb_wrapper
- filter_prepare_cb_wrapper
- filter_by_origin_cb_wrapper

Attach the new patch.

[1] - https://www.postgresql.org/message-id/20230213180302.u5sqosteflr3zkiz%40awork3.anarazel.de

Regards,
Wang wei