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: Peter Smith <smithpb2250@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2020-11-25T12:55:48Z
Lists: pgsql-hackers

Attachments

On Tue, Nov 24, 2020 at 3:29 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Mon, Nov 23, 2020 at 10:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > For the first two, as the xact is still not visible to others so we
> > don't need to make it behave like a committed txn. To make the (DDL)
> > changes visible to the current txn, the message
> > REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID copies the snapshot which
> > fills the subxip array. This will be sufficient to make the changes
> > visible to the current txn. For the third, I have checked the code
> > that whenever we have any change message the base snapshot gets set
> > via SnapBuildProcessChange. It is possible that I have missed
> > something but I don't want to call SnapbuildCommittedTxn in
> > DecodePrepare unless we have a clear reason for the same so leaving it
> > for now. Can you or someone see any reason for the same?
>
> I reviewed and tested this and like you said, SnapBuildProcessChange
> sets the base snapshot for every change.
> I did various tests using DDL updates and haven't seen any issues so
> far. I agree with your analysis.
>

Thanks, attached is a further revised version of the patch series.

Changes in v27-0001-Extend-the-output-plugin-API-to-allow-decoding-p
a. Removed the includes which are not required by this patch.
b. Moved the 'check_xid_aborted' parameter to 0004.
c. Added Assert(!ctx->fast_forward); in callback wrappers, because we
won't load the output plugin when fast_forward is set so there is no
chance that we call output plugin APIs. This is why we have this
Assert in all the existing APIs.
d. Adjusted the order of various callback APIs to make the code look consistent.
e. Added/Edited comments and doc updates at various places. Changed
error messages to make them consistent with other similar messages.
f. Some other cosmetic changes like the removal of spurious new lines
and fixed white-space issues.
g. Updated commit message.

Changes in v27-0002-Allow-decoding-at-prepare-time-in-ReorderBuffer
a. Move the check to whether a particular txn can be skipped into a
separate function as the same code for it was repeated at three
different places.
b. ReorderBufferPrepare has a parameter name as commit_lsn whereas it
should be preapre_lsn. Similar changes has been made at various places
in the patch.
c. filter_prepare_cb callback existence is checked in both decode.c
and in filter_prepare_cb_wrapper. Fixed by removing it from decode.c.
d. Fixed miscellaneous comments and some cosmetic changes.
e. Moved the special elog in ReorderBufferProcessTxn to test
concurrent aborts in 0004 patch.
f. Moved the changes related to flags RBTXN_COMMIT_PREPARED and
RBTXN_ROLLBACK_PREPARED to patch 0006 as those are used only in that
patch.
g. Updated commit message.

One problem with this patch is: What if we have assembled a consistent
snapshot after prepare and before commit prepared. In that case, it
will currently just send commit prepared record which would be a bad
idea. It should decode the entire transaction for such cases at commit
prepared time. This same problem is noticed by Arseny Sher, see
problem-2 in email [1].

One idea to fix this could be to check if the snapshot is consistent
to decide whether to skip the prepare and if we skip due to that
reason, then during commit we need to decode the entire transaction.
We can do that by setting a flag in txn->txn_flags such that during
prepare we can set a flag when we skip the prepare because the
snapshot is still not consistent and then used it during commit to see
if we need to decode the entire transaction. But here we need to think
about what would happen after restart? Basically, if it is possible
that after restart the snapshot is consistent for the same transaction
at prepare time and it got skipped due to start_decoding_at (which
moved ahead after restart) then such a solution won't work. Any
thoughts on this?

v27-0004-Support-2PC-txn-tests-for-concurrent-aborts
a. Moved the changes related to testing of concurrent aborts in this
patch from other patches.

v27-0006-Support-2PC-txn-pgoutput
a. Moved the changes related to flags RBTXN_COMMIT_PREPARED and
RBTXN_ROLLBACK_PREPARED from other patch.
b. Included headers required by this patch, previously it seems to be
dependent on other patches for this.

The other patches remain unchanged.

Let me know what you think about these changes?

[1] - https://www.postgresql.org/message-id/877el38j56.fsf%40ars-thinkpad

-- 
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