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. Regards, -- Fujii Masao
002_Reset_on_commit_launcher_wakeup_v5.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
