Horton

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

Amit Kapila <amit.kapila16@gmail.com>

View thread View on postgr.es
From: Amit Kapila <amit.kapila16@gmail.com>
To: Ajin Cherian <itsajin@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, Masahiko Sawada <sawada.mshk@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2020-09-29T13:16:03Z
Lists: pgsql-hackers 
On Tue, Sep 29, 2020 at 5:08 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, Sep 23, 2020 at 2:39 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > If the transaction is prepared which you can ensure via
> > ReorderBufferTxnIsPrepared() (considering you have a proper check in
> > that function), it should not require skipping the transaction in
> > Abort. One way it could happen is if you clean up the ReorderBufferTxn
> > in Prepare which you were doing in earlier version of patch which I
> > pointed out was wrong, if you have changed that then I don't know why
> > it could fail, may be someplace else during prepare the patch is
> > freeing it. Just check that.
>
> I had a look at this problem. The problem happens when decoding is
> done after a prepare but before the corresponding rollback
> prepared/commit prepared.
> For eg:
>
> Begin;
> <change 1>
> <change 2>
> PREPARE TRANSACTION '<prepare#1>';
> SELECT data FROM pg_logical_slot_get_changes(...);
> :
> :
> ROLLBACK PREPARED '<prepare#1>';
> SELECT data FROM pg_logical_slot_get_changes(...);
>
> Since the prepare is consumed in the first call to
> pg_logical_slot_get_changes, subsequently when it is encountered in
> the second call, it is skipped (as already decoded) in DecodePrepare
> and the txn->flags are not set to
> reflect the fact that it was prepared. The same behaviour is seen when
> it is commit prepared after the original prepare was consumed.
> Initially I was thinking about the following approach to fix it in DecodePrepare
> Approach 1:
>        1. Break the big Skip check in DecodePrepare into 2 parts.
>           Return if the following conditions are true:
>          If (parsed->dbId != InvalidOid && parsed->dbId !=
> ctx->slot->data.database) ||
>            ctx->fast_forward || FilterByOrigin(ctx, origin_id))
>
>       2. Check If this condition is true:
>           SnapBuildXactNeedsSkip(ctx->snapshot_builder, buf->origptr)
>
>           Then this means we are skipping because this has already
> been decoded, then instead of returning, call a new function
> ReorderBufferMarkPrepare() which will only update the flags in the txn
> to indicate that the transaction is prepared
>           Then later in DecodeAbort or DecodeCommit, we can confirm
> that the transaction has been Prepared by checking if the flag is set
> and call ReorderBufferFinishPrepared appropriately.
>
> But then, thinking about this some more, I thought of a second approach.
> Approach 2:
>         If the only purpose of all this was to differentiate between
> Abort vs Rollback Prepared and Commit vs Commit Prepared, then we dont
> need this. We already know the exact operation
>         in DecodeXactOp and can differentiate there. We only
> overloaded DecodeAbort and DecodeCommit for convenience, we can always
> call these functions with an extra flag to denote that we are either
> commit or aborting a
>         previously prepared transaction and call
> ReorderBufferFinishPrepared accordingly.
>

The second approach sounds better but you can see if there is not much
you want to reuse from DecodeCommit/DecodeAbort then you can even
write new functions DecodeCommitPrepared/DecodeAbortPrepared. OTOH, if
there is a common code among them then passing the flag would be a
better way.

-- 
With Regards,
Amit Kapila.

Commits

  1. Add prepare API support for streaming transactions in logical replication.

  2. Doc: minor improvements for logical replication protocol documentation.

  3. Fix test failure in 021_twophase.pl.

  4. Refactor to make common functions in proto.c and worker.c.

  5. Unify PostgresNode's new() and get_new_node() methods

  6. Fix potential buffer overruns in proto.c.

  7. Add support for prepared transactions to built-in logical replication.

  8. Refactor function parse_subscription_options.

  9. Allow enabling two-phase option via replication protocol.

  10. Don't use Asserts to check for violations of replication protocol.

  11. Improve psql tab completion for options of subcriptions and publications

  12. Rearrange logrep worker's snapshot handling some more.

  13. doc: Update information of new messages for logical replication.

  14. ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION

  15. Allow pgoutput to send logical decoding messages.

  16. Refactor function parse_output_parameters.

  17. Avoid repeated decoding of prepared transactions after a restart.

  18. Fix an oversight in ReorderBufferFinishPrepared.

  19. Allow multiple xacts during table sync in logical replication.

  20. Fix replication of in-progress transactions in tablesync worker.

  21. Fix 'skip-empty-xacts' option in test_decoding for streaming mode.

  22. Use Enum for top level logical replication message types.

  23. Add support for streaming to built-in logical replication.

  24. Fix the logical streaming test.

  25. Implement streaming mode in ReorderBuffer.

  26. Extend the logical decoding output plugin API with stream methods.

  27. Store 2PC GID in commit/abort WAL recs for logical decoding