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: "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>, Peter Smith <smithpb2250@gmail.com>, vignesh C <vignesh21@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2021-05-28T06:25:38Z
Lists: pgsql-hackers 
On Thu, May 27, 2021 at 8:05 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> Thanks for confirmation. The problem seemed to be as you reported a
> table not closed when a transaction was committed.
> This seems to be because the function UpdateTwoPhaseState was
> committing a transaction inside the function when the caller of
> UpdateTwoPhaseState had
> a table open in CreateSubscription. This function was newly included
> in the CreateSubscription code, to handle the new use case of
> two_phase being enabled on
> create subscription if "copy_data = false". I don't think
> CreateSubscription required this to be inside a transaction and the
> committing of transaction
> was only meant for where this function was originally created to be
> used in the apply worker code (ApplyWorkerMain()).
> So, I removed the committing of the transaction from inside the
> function UpdateTwoPhaseState() and instead started and committed the
> transaction
>  prior to and after this function is invoked in the apply worker code.
>

You have made these changes in 0002 whereas they should be part of 0001.

One minor comment for 0001.
* Special case: if when tables were specified but copy_data is
+ * false then it is safe to enable two_phase up-front because
+ * those tables are already initially READY state. Note, if
+ * the subscription has no tables then enablement cannot be
+ * done here - we must leave the twophase state as PENDING, to
+ * allow ALTER SUBSCRIPTION ... REFRESH PUBLICATION to work.

Can we slightly modify this comment as: "Note that if tables were
specified but copy_data is false then it is safe to enable two_phase
up-front because those tables are already initially READY state. When
the subscription has no tables, we leave the twophase state as
PENDING, to allow ALTER SUBSCRIPTION ... REFRESH PUBLICATION to work."

Also, I don't see any test after you enable this special case. Is it
covered by existing tests, if not then let's try to add a test for
this?

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