Re: [HACKERS] logical decoding of two-phase transactions

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