On 2018-08-06 21:06:13 +0300, Arseny Sher wrote: > Hello, > > I have looked through the patches. I will first describe relativaly > serious issues I see and then proceed with small nitpicking. > > - On decoding of aborted xacts. The idea to throw an error once we > detect the abort is appealing, however I think you will have problems > with subxacts in the current implementation. What if subxact issues > DDL and then aborted, but main transaction successfully committed?
I don't see a fundamental issue here. I've not reviewed the current patchset meaningfully, however. Do you see a fundamental issue here? > - Decoding transactions at PREPARE record changes rules of the "we ship > all commits after lsn 'x'" game. Namely, it will break initial > tablesync: what if consistent snapshot was formed *after* PREPARE, but > before COMMIT PREPARED, and the plugin decides to employ 2pc? Instead > of getting inital contents + continious stream of changes the receiver > will miss the prepared xact contents and raise 'prepared xact doesn't > exist' error. I think the starting point to address this is to forbid > two-phase decoding of xacts with lsn of PREPARE less than > snapbuilder's start_decoding_at. > Yea, that sounds like it need to be addressed. > - Currently we will call abort_prepared cb even if we failed to actually > prepare xact due to concurrent abort. I think it is confusing for > users. We should either handle this by remembering not to invoke > abort_prepared in these cases or at least document this behaviour, > leaving this problem to the receiver side. What precisely do you mean by "concurrent abort"? > - I find it suspicious that DecodePrepare completely ignores actions of > SnapBuildCommitTxn. For example, to execute invalidations, the latter > sets base snapshot if our xact (or subxacts) did DDL and the snapshot > not set yet. My fantasy doesn't hint me the concrete example > where this would burn at the moment, but it should be considered. Yea, I think this need to mirror the actions (and thus generalize the code to avoid duplication) > Now, the bikeshedding. > > First patch: > - I am one of those people upthread who don't think that converting > flags to bitmask is beneficial -- especially given that many of them > are mutually exclusive, e.g. xact can't be committed and aborted at > the same time. Apparently you have left this to the committer though. Similar. - Andres
