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
