On Thu, Oct 20, 2022 at 2:08 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 7. get_transaction_apply_action
>
> > 12. get_transaction_apply_action
> >
> > I still felt like there should be some tablesync checks/comments in
> > this function, just for sanity, even if it works as-is now.
> >
> > For example, are you saying ([3] #22b) that there might be rare cases
> > where a Tablesync would call to parallel_apply_find_worker? That seems
> > strange, given that "for streaming transactions that are being applied
> > in the parallel ... we disallow applying changes on a table that is
> > not in the READY state".
> >
> > ------
>
> Houz wrote [2] -
>
> I think because we won't try to start parallel apply worker in table sync
> worker(see the check in parallel_apply_can_start()), so we won't find any
> worker in parallel_apply_find_worker() which means get_transaction_apply_action
> will return TRANS_LEADER_SERIALIZE. And get_transaction_apply_action is a
> function which can be invoked for all kinds of workers(same is true for all
> apply_handle_xxx functions), so not sure if table sync check/comment is
> necessary.
>
> ~
>
> Sure, and I believe you when you say it all works OK - but IMO there
> is something still not quite right with this current code. For
> example,
>
> e.g.1 the functional will return TRANS_LEADER_SERIALIZE for Tablesync
> worker, and yet the comment for TRANS_LEADER_SERIALIZE says "means
> that we are in the leader apply worker" (except we are not)
>
> e.g.2 we know for a fact that Tablesync workers cannot start their own
> parallel apply workers, so then why do we even let the Tablesync
> worker make a call to parallel_apply_find_worker() looking for
> something we know will not be found?
>
I don't see much benefit in adding an additional check for tablesync
workers here. It will unnecessarily make this part of the code look
bit ugly.
--
With Regards,
Amit Kapila.
