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: Peter Smith <smithpb2250@gmail.com>
Cc: Ajin Cherian <itsajin@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2021-02-10T13:46:06Z
Lists: pgsql-hackers 
On Wed, Feb 10, 2021 at 3:59 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> PSA the new patch set v38*.
>
> This patch set has been rebased to use the most recent tablesync patch
> from other thread [1]
> (i.e. notice that v38-0001 is an exact copy of that thread's tablesync
> patch v31)
>

I see one problem which might lead to the skip of prepared xacts for
some of the subscriptions. The problem is that we skip the prepared
xacts based on GID and the same prepared transaction arrives on the
subscriber for different subscriptions. And even if we wouldn't have
skipped the prepared xact, it would have lead to an error "transaction
identifier "p1" is already in use". See the scenario below:

On Publisher:
===========
CREATE TABLE mytbl1(id SERIAL PRIMARY KEY, somedata int, text varchar(120));
CREATE TABLE mytbl2(id SERIAL PRIMARY KEY, somedata int, text varchar(120));
postgres=# BEGIN;
BEGIN
postgres=*# INSERT INTO mytbl1(somedata, text) VALUES (1, 1);
INSERT 0 1
postgres=*# INSERT INTO mytbl1(somedata, text) VALUES (1, 2);
INSERT 0 1
postgres=*# COMMIT;
COMMIT
postgres=# BEGIN;
BEGIN
postgres=*# INSERT INTO mytbl2(somedata, text) VALUES (1, 1);
INSERT 0 1
postgres=*# INSERT INTO mytbl2(somedata, text) VALUES (1, 2);
INSERT 0 1
postgres=*# Commit;
COMMIT
postgres=# CREATE PUBLICATION mypub1 FOR TABLE mytbl1;
CREATE PUBLICATION
postgres=# CREATE PUBLICATION mypub2 FOR TABLE mytbl2;
CREATE PUBLICATION

On Subscriber:
============
CREATE TABLE mytbl1(id SERIAL PRIMARY KEY, somedata int, text varchar(120));
CREATE TABLE mytbl2(id SERIAL PRIMARY KEY, somedata int, text varchar(120));
postgres=# CREATE SUBSCRIPTION mysub1
postgres-#          CONNECTION 'host=localhost port=5432 dbname=postgres'
postgres-#         PUBLICATION mypub1;
NOTICE:  created replication slot "mysub1" on publisher
CREATE SUBSCRIPTION
postgres=# CREATE SUBSCRIPTION mysub2
postgres-#          CONNECTION 'host=localhost port=5432 dbname=postgres'
postgres-#         PUBLICATION mypub2;
NOTICE:  created replication slot "mysub2" on publisher
CREATE SUBSCRIPTION

On Publisher:
============
postgres=# Begin;
BEGIN
postgres=*# INSERT INTO mytbl1(somedata, text) VALUES (1, 3);
INSERT 0 1
postgres=*# INSERT INTO mytbl2(somedata, text) VALUES (1, 3);
INSERT 0 1
postgres=*# Prepare Transaction 'myprep1';

After this step, wait for few seconds and then perform Commit Prepared
'myprep1'; on Publisher and you will notice following error in the
subscriber log:  "ERROR:  prepared transaction with identifier
"myprep1" does not exist"

One idea to avoid this is that we use subscription_id along with GID
on subscription for prepared xacts. Let me know if you have any better
ideas to handle this?

Few other minor comments on
v38-0004-Add-support-for-apply-at-prepare-time-to-built-i:
======================================================================
1.
- * Mark the prepared transaction as valid.  As soon as xact.c marks
- * MyProc as not running our XID (which it will do immediately after
- * this function returns), others can commit/rollback the xact.
+ * Mark the prepared transaction as valid.  As soon as xact.c marks MyProc
+ * as not running our XID (which it will do immediately after this
+ * function returns), others can commit/rollback the xact.

Why this change in this patch? Is it due to pgindent? If so, you need
to exclude this change?

2.
@@ -78,7 +78,7 @@ logicalrep_write_commit(StringInfo out, ReorderBufferTXN *txn,

  pq_sendbyte(out, LOGICAL_REP_MSG_COMMIT);

- /* send the flags field (unused for now) */
+ /* send the flags field */
  pq_sendbyte(out, flags);

Is there a reason to change the above comment?

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