Re: [HACKERS] logical decoding of two-phase transactions
Amit Kapila <amit.kapila16@gmail.com>
On Tue, Jul 27, 2021 at 11:41 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Please find attached the latest patch set v99*
>
> v98-0001 --> split into v99-0001 + v99-0002
>
Pushed the first refactoring patch after making few modifications as below.
1.
- /* open the spool file for the committed transaction */
+ /* Open the spool file for the committed/prepared transaction */
changes_filename(path, MyLogicalRepWorker->subid, xid);
In the above comment, we don't need to say prepared. It can be done as
part of the second patch.
2.
+apply_handle_prepare_internal(LogicalRepPreparedTxnData
*prepare_data, char *gid)
I don't think there is any need for this function to take gid as
input. It can compute by itself instead of callers doing it.
3.
+static TransactionId+logicalrep_read_prepare_common(StringInfo in,
char *msgtype,
+ LogicalRepPreparedTxnData *prepare_data)
I don't think the above function needs to return xid because it is
already present as part of prepare_data. Even, if it is required due
to some reason for the second patch then let's do it as part of if but
I don't think it is required for the second patch.
4.
/*
- * Write PREPARE to the output stream.
+ * Common code for logicalrep_write_prepare and
logicalrep_write_stream_prepare.
*/
Here and at a similar another place, we don't need to refer to
logicalrep_write_stream_prepare as that is part of the second patch.
Few comments on 0002 patch:
==========================
1.
+# ---------------------
+# 2PC + STREAMING TESTS
+# ---------------------
+
+# Setup logical replication (streaming = on)
+
+$node_B->safe_psql('postgres', "
+ ALTER SUBSCRIPTION tap_sub_B
+ SET (streaming = on);");
+
+$node_C->safe_psql('postgres', "
+ ALTER SUBSCRIPTION tap_sub_C
+ SET (streaming = on)");
+
+# Wait for subscribers to finish initialization
+$node_A->wait_for_catchup($appname_B);
+$node_B->wait_for_catchup($appname_C);
This is not the right way to determine if the new streaming option is
enabled on the publisher. Even if there is no restart of apply workers
(and walsender) after you have enabled the option, the above wait will
succeed. You need to do something like below as we are doing in
001_rep_changes.pl:
$oldpid = $node_publisher->safe_psql('postgres',
"SELECT pid FROM pg_stat_replication WHERE application_name = 'tap_sub';"
);
$node_subscriber->safe_psql('postgres',
"ALTER SUBSCRIPTION tap_sub SET PUBLICATION tap_pub_ins_only WITH
(copy_data = false)"
);
$node_publisher->poll_query_until('postgres',
"SELECT pid != $oldpid FROM pg_stat_replication WHERE application_name
= 'tap_sub';"
) or die "Timed out while waiting for apply to restart";
2.
+# Create some pre-existing content on publisher (uses same DDL as
015_stream test)
Here, in the comments, I don't see the need to same uses same DDL ...
--
With Regards,
Amit Kapila.
Commits
-
Add prepare API support for streaming transactions in logical replication.
- 63cf61cdeb7b 15.0 landed
-
Doc: minor improvements for logical replication protocol documentation.
- a5cb4f9829fb 15.0 cited
-
Fix test failure in 021_twophase.pl.
- eaf5321c3524 15.0 landed
-
Refactor to make common functions in proto.c and worker.c.
- 91f9861242cd 15.0 landed
-
Unify PostgresNode's new() and get_new_node() methods
- 201a76183e20 15.0 cited
-
Fix potential buffer overruns in proto.c.
- 01c3adcdd85f 15.0 landed
-
Add support for prepared transactions to built-in logical replication.
- a8fd13cab0ba 15.0 cited
-
Refactor function parse_subscription_options.
- 8aafb0261675 15.0 cited
-
Allow enabling two-phase option via replication protocol.
- cda03cfed6b8 15.0 cited
-
Don't use Asserts to check for violations of replication protocol.
- fe6a20ce54cb 14.0 cited
-
Improve psql tab completion for options of subcriptions and publications
- d08237b5b494 14.0 cited
-
Rearrange logrep worker's snapshot handling some more.
- 3a09d75b4f6c 14.0 cited
-
doc: Update information of new messages for logical replication.
- 15c1a9d9cb76 14.0 cited
-
ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION
- 82ed7748b710 14.0 cited
-
Allow pgoutput to send logical decoding messages.
- ac4645c0157f 14.0 cited
-
Refactor function parse_output_parameters.
- 531737ddad21 14.0 cited
-
Avoid repeated decoding of prepared transactions after a restart.
- 8bdb1332eb51 14.0 cited
-
Fix an oversight in ReorderBufferFinishPrepared.
- ade89ba5f408 14.0 landed
-
Allow multiple xacts during table sync in logical replication.
- ce0fdbfe9722 14.0 cited
-
Fix replication of in-progress transactions in tablesync worker.
- 0926e96c4934 14.0 cited
-
Fix 'skip-empty-xacts' option in test_decoding for streaming mode.
- 9653f24ad830 14.0 cited
-
Use Enum for top level logical replication message types.
- 644f0d7cc9c2 14.0 cited
-
Add support for streaming to built-in logical replication.
- 464824323e57 14.0 cited
-
Fix the logical streaming test.
- 82a0ba7707e0 14.0 cited
-
Implement streaming mode in ReorderBuffer.
- 7259736a6e5b 14.0 cited
-
Extend the logical decoding output plugin API with stream methods.
- 45fdc9738b36 14.0 cited
-
Store 2PC GID in commit/abort WAL recs for logical decoding
- 1eb6d6527aae 11.0 cited