On Tue, Mar 7, 2023 15:55 PM Kuroda, Hayato/黒田 隼人 <kuroda.hay...@fujitsu.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
v6-0001-Rework-LogicalOutputPluginWriterUpdateProgress.patch
Description: v6-0001-Rework-LogicalOutputPluginWriterUpdateProgress.patch
