Horton

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

Ajin Cherian <itsajin@gmail.com>

View thread View on postgr.es
From: Ajin Cherian <itsajin@gmail.com>
To: Amit Kapila <amit.kapila16@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-29T11:37:59Z
Lists: pgsql-hackers 
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.

Let me know your thoughts on these two approaches or any other
suggestions on this.

regards,
Ajin Cherian
Fujitsu Australia

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