On Wed, Apr 26, 2017 at 12:35 PM, Petr Jelinek <petr.jeli...@2ndquadrant.com> wrote: > On 26/04/17 01:01, Fujii Masao wrote: >> On Mon, Apr 24, 2017 at 7:57 PM, Kyotaro HORIGUCHI >> <horiguchi.kyot...@lab.ntt.co.jp> wrote: >>> Hello, >>> >>> At Mon, 24 Apr 2017 11:18:32 +0900, Masahiko Sawada <sawada.m...@gmail.com> >>> wrote in >>> <cad21aobu8mzdgx-stj3u+qkaej5rpnouotw4kfexc4xddnf...@mail.gmail.com> >>>>>> BEGIN; >>>>>> ALTER SUBSCRIPTION hoge_sub ENABLE; >>>>>> PREPARE TRANSACTION 'g'; >>>>>> BEGIN; >>>>>> SELECT 1; >>>>>> COMMIT; -- wake up the launcher at this point. >>>>>> COMMIT PREPARED 'g'; >>>>>> >>>>>> Even if we wake up the launcher even when a ROLLBACK PREPARED, it's >>>>>> not a big deal to the launcher process actually. Compared with the >>>>>> complexly of changing the logic so that on_commit_launcher_wakeup >>>>>> corresponds to prepared transaction, we might want to accept this >>>>>> behavior. >>>>> >>>>> on_commit_launcher_wakeup needs to be recoreded in 2PC state >>>>> file to handle this issue properly. But I agree with you, that would >>>>> be overkill for small gain. So I'm thinking to add the following >>>>> comment into your patch and push it. Thought? >>>>> >>>>> ------------------------- >>>>> Note that on_commit_launcher_wakeup flag doesn't work as expected in 2PC >>>>> case. >>>>> For example, COMMIT PREPARED on the transaction enabling the flag >>>>> doesn't wake up >>>>> the logical replication launcher if ROLLBACK on another transaction >>>>> runs before it. >>>>> To handle this case properly, the flag needs to be recorded in 2PC >>>>> state file so that >>>>> COMMIT PREPARED and ROLLBACK PREPARED can determine whether to wake up >>>>> the launcher. However this is overkill for small gain and false wakeup >>>>> of the launcher >>>>> is not so harmful (probably we can live with that), so we do nothing >>>>> here for this issue. >>>>> ------------------------- >>>>> >>>> >>>> Agreed. >>>> >>>> I added this comment to the patch and attached it. >>> >>> The following "However" may need a follwoing comma. >>> >>>> However this is overkill for small gain and false wakeup of the >>>> launcher is not so harmful (probably we can live with that), so >>>> we do nothing here for this issue. >>> >>> I agree this as a whole. But I think that the main issue here is >>> not false wakeups, but 'possible delay of launching new workers >>> by 3 minutes at most' (this is centainly a kind of false wakeups, >>> though). We can live with this failure when using two-paase >>> commmit, but I think it shouldn't happen silently. >>> >>> >>> How about providing AtPrepare_ApplyLauncher(void) like the >>> follows and calling it in PrepareTransaction? >> >> Or we should apply the attached patch and handle the 2PC case properly? >> I was thinking that it's overkill more than necessary, but that seems not >> true >> as far as I implement that. >>
In my honest opinion, I didn't have a big will that we should handle even two-phase commit case, because this case is very rare (I could not image such case) and doesn't mean to lead a harmful result such as crash of server and returning inconsistent result. it just delays the launching worker for at most 3 minutes. We also can deal with this for example by making maximum nap time of apply launcher user-configurable and document it. But if we can deal with it by minimum changes like attached your patch I agree. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
