On Thursday, October 20, 2022 5:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 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.
Thanks for the review, here is the new version patch set which addressed Peter[1]
and Kuroda-san[2]'s comments.
[1] https://www.postgresql.org/message-id/CAHut%2BPs0HXawMD%3DzQ5YUncc9kjGy%2Bmd_39Y4Fdf%3DsKjt-LE92g%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/TYAPR01MB586674C1EE91C06DBACE7728F52B9%40TYAPR01MB5866.jpnprd01.prod.outlook.com
Best regards,
Hou zj
