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. >
Looks like it does not even increase size of the 2pc file, +1 for this. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
