Horton

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

vignesh C <vignesh21@gmail.com>

View thread View on postgr.es
From: vignesh C <vignesh21@gmail.com>
To: Ajin Cherian <itsajin@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2021-03-05T10:25:25Z
Lists: pgsql-hackers 
On Fri, Mar 5, 2021 at 12:21 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Thu, Mar 4, 2021 at 9:53 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> > [05a] Now syncing the psf file at prepare time
>
> The patch v46-0008 does not handle spooling of streaming prepare if
> the Subscription is configured for both two-phase and streaming.
> I feel that it would be best if we don't support both two-phase and
> streaming together in a subscription in this release.
> Probably a future release could handle this. So, changing the patch to
> not allow streaming and two-phase together.
> This new patch v49 has the following changes.
>
> * Don't support creating a subscription with both streaming and
> two-phase enabled.
> * Don't support altering a subscription enabling streaming if it was
> created with two-phase enabled.
> * Remove stream_prepare callback as a "required" callback, make it an
> optional callback and remove all code related to stream_prepare in the
> pgoutput plugin as well as in worker.c
>
> Also fixed
> * Don't support the alter of subscription setting two-phase. Toggling
> of two-phase mode using the alter command on the subscription can
> cause transactions to be missed and result in an inconsistent replica.
>

Thanks for the updated patch.
Few minor comments:

I'm not sure if we plan to change this workaround, if we are not
planning to change this workaround. We can reword the comments
suitably. We generally don't use workaround in our comments.
+               /*
+                * Workaround Part 1 of 2:
+                *
+                * Make sure every tablesync has reached at least SYNCDONE state
+                * before letting the apply worker proceed.
+                */
+               elog(DEBUG1,
+                        "apply_handle_begin_prepare, end_lsn = %X/%X,
final_lsn = %X/%X, lstate_lsn = %X/%X",
+                        LSN_FORMAT_ARGS(begin_data.end_lsn),
+                        LSN_FORMAT_ARGS(begin_data.final_lsn),
+                        LSN_FORMAT_ARGS(MyLogicalRepWorker->relstate_lsn));
+

We should include two_phase in tab completion (tab-complete.c file
psql_completion(const char *text, int start, int end) function) :
postgres=# create subscription sub1 connection 'port=5441
dbname=postgres' publication  pub1 with (
CONNECT             COPY_DATA           CREATE_SLOT         ENABLED
         SLOT_NAME           SYNCHRONOUS_COMMIT

+
+         <para>
+          It is not allowed to combine <literal>streaming</literal> set to
+          <literal>true</literal> and <literal>two_phase</literal> set to
+          <literal>true</literal>.
+         </para>
+
+        </listitem>
+       </varlistentry>
+       <varlistentry>
+        <term><literal>two_phase</literal> (<type>boolean</type>)</term>
+        <listitem>
+         <para>
+          Specifies whether two-phase commit is enabled for this subscription.
+          The default is <literal>false</literal>.
+         </para>
+
+         <para>
+          When two-phase commit is enabled then the decoded
transactions are sent
+          to the subscriber on the PREPARE TRANSACTION. By default,
the transaction
+          preapred on publisher is decoded as normal transaction at commit.
+         </para>
+
+         <para>
+          It is not allowed to combine <literal>two_phase</literal> set to
+          <literal>true</literal> and <literal>streaming</literal> set to
+          <literal>true</literal>.
+         </para>

It is not allowed to combine streaming set to true and two_phase set to true.
Should this be:
streaming option is not supported along with two_phase option.

Similarly here too:
It is not allowed to combine two_phase set to true and streaming set to true.
Should this be:
two_phase option is not supported along with streaming option.

Few indentation issues are present, we can run pgindent:
+extern void logicalrep_write_prepare(StringInfo out, ReorderBufferTXN *txn,
+
  XLogRecPtr prepare_lsn);
+extern void logicalrep_read_prepare(StringInfo in,
+
 LogicalRepPreparedTxnData *prepare_data);
+extern void logicalrep_write_commit_prepared(StringInfo out,
ReorderBufferTXN* txn,
+
                  XLogRecPtr commit_lsn);

ReorderBufferTXN * should be ReorderBufferTXN*

Line exceeds 80 chars:
+               /*
+                * Now that we replayed the psf it is no longer
needed. Just delete it.
+                */
+               prepare_spoolfile_delete(psfpath);

There is a typo, preapred should be prepared.
+         <para>
+          When two-phase commit is enabled then the decoded
transactions are sent
+          to the subscriber on the PREPARE TRANSACTION. By default,
the transaction
+          preapred on publisher is decoded as normal transaction at commit.
+         </para>

Regards,
Vignesh

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