On Thu, Oct 20, 2022 at 2:08 PM Peter Smith <smithpb2...@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.
