On Wed, Nov 25, 2020 at 11:20 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Mon, Nov 23, 2020 at 8:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> REVIEW COMMENTS
>
> Despite the tests working OK
>
Thanks for the tests.
> I thought there should be a couple small
> changes made for this patch, as follows.
>
> (1) process_sync_tables should be last
>
> IMO the process_syncing_tables should be always the *last* thing any
> apply handler does, because it seems a bad idea for the worker to
> change state (e.g. to say it is SUBREL_STATE_SYNCDONE) if in fact
> there is still a little bit more processing remaining.
>
Hmm, what makes you think it is a bad idea, is there any comment which
makes you feel so? As far as I understand, the only thing tablesync
worker needs to ensure before reaching that state is it should apply
till the required lsn which is done by the time it is called in the
patch. I think doing it sooner is better because it will let apply
worker start its work. In any case, this is not related to this
bug-fix patch, so, if you want to build a case for doing it later then
we can discuss it separately.
>
> (2) misleading comment
>
> /*
> * Start a transaction on stream start, this transaction will be committed
> * on the stream stop. We need the transaction for handling the buffile,
> * used for serializing the streaming data and subxact info.
> */
>
> That above comment (in the apply_handle_stream_start function) may now
> be inaccurate/misleading for the case of tablesync worker, because I
> think for tablesync you are explicitly avoiding the tx commit within
> the apply_handle_stream_stop().
>
Okay, I think we can slightly adjust the comment as: "Start a
transaction on stream start, this transaction will be committed on the
stream stop unless it is a tablesync worker in which case it will be
committed after processing all the messages. We need the transaction
for handling the buffile, used for serializing the streaming data and
subxact info.".
I can update the patch once we agree on the previous point.
--
With Regards,
Amit Kapila.