From: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | "wangw(dot)fnst(at)fujitsu(dot)com" <wangw(dot)fnst(at)fujitsu(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Peter Smith <smithpb2250(at)gmail(dot)com> |
Subject: | RE: Perform streaming logical transactions by background workers and parallel apply |
Date: | 2022-10-21 09:32:20 |
Message-ID: | OS0PR01MB57163F73F6FA5741F3FF2176942D9@OS0PR01MB5716.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thursday, October 20, 2022 5:49 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Thu, Oct 20, 2022 at 2:08 PM Peter Smith <smithpb2250(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
v40-0006-Add-a-main_worker_pid-to-pg_stat_subscription.patch | application/octet-stream | 8.7 KB |
v40-0001-Perform-streaming-logical-transactions-by-parall.patch | application/octet-stream | 144.0 KB |
v40-0002-Test-streaming-parallel-option-in-tap-test.patch | application/octet-stream | 74.5 KB |
v40-0003-Add-some-checks-before-using-parallel-apply-work.patch | application/octet-stream | 49.7 KB |
v40-0004-Retrict-parallel-for-partitioned-table.patch | application/octet-stream | 18.5 KB |
v40-0005-Retry-to-apply-streaming-xact-only-in-apply-work.patch | application/octet-stream | 61.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Kyotaro Horiguchi | 2022-10-21 09:38:06 | Re: Standby recovers records from wrong timeline |
Previous Message | Alvaro Herrera | 2022-10-21 09:32:16 | parse partition strategy string in gram.y |