On 18.01.22 07:05, Masahiko Sawada wrote:
I've attached a rebased patch.
I think this is now almost done. Attached I have a small fixup patch
with some documentation proof-reading, and removing some comments I felt
are redundant. Some others have also sent you some documentation
updates, so feel free to merge mine in with them.
Some other comments:
parse_subscription_options() and AlterSubscriptionStmt mixes regular
options and skip options in ways that confuse me. It seems to work
correctly, though. I guess for now it's okay, but if we add more skip
options, it might be better to separate those more cleanly.
I think the superuser check in AlterSubscription() might no longer be
appropriate. Subscriptions can now be owned by non-superusers. Please
check that.
The display order in psql \dRs+ is a bit odd. I would put it at the
end, certainly not between Two phase commit and Synchronous commit.
Please run pgperltidy over 028_skip_xact.pl.
Is the setting of logical_decoding_work_mem in the test script required?
If so, comment why.
Please document arguments origin_lsn and origin_timestamp of
stop_skipping_changes(). Otherwise, one has to dig quite deep to find
out what they are for.
This is all minor stuff, so I think when this and the nearby comments
are addressed, this is fine by me.From 751be8216317f1d996c7b4f9f0e915adff805567 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Thu, 20 Jan 2022 17:03:27 +0100
Subject: [PATCH] fixup! Add ALTER SUBSCRIPTION ... SKIP to skip the
transaction on subscriber nodes
---
doc/src/sgml/logical-replication.sgml | 12 ++++--------
doc/src/sgml/ref/alter_subscription.sgml | 16 ++++++++--------
src/backend/commands/subscriptioncmds.c | 2 +-
src/backend/replication/logical/worker.c | 7 -------
src/test/regress/expected/subscription.out | 6 +++---
src/test/subscription/t/028_skip_xact.pl | 8 +++++---
6 files changed, 21 insertions(+), 30 deletions(-)
diff --git a/doc/src/sgml/logical-replication.sgml
b/doc/src/sgml/logical-replication.sgml
index de4f83bbcc..873530db99 100644
--- a/doc/src/sgml/logical-replication.sgml
+++ b/doc/src/sgml/logical-replication.sgml
@@ -355,11 +355,10 @@ <title>Conflicts</title>
The resolution can be done either by changing data or permissions on the
subscriber so
that it does not conflict with the incoming changes or by skipping the
the transaction that conflicts with the existing data. When a conflict
- produces an error, it is shown in
+ produces an error, it is shown in the
<structname>pg_stat_subscription_workers</structname> view as follows:
- </para>
- <programlisting>
+<programlisting>
postgres=# SELECT * FROM pg_stat_subscription_workers;
-[ RECORD 1 ]------+-----------------------------------------------------------
subid | 16391
@@ -373,9 +372,7 @@ <title>Conflicts</title>
last_error_time | 2021-09-29 15:52:45.165754+00
</programlisting>
- <para>
and it is also shown in subscriber's server log as follows:
- </para>
<screen>
ERROR: duplicate key value violates unique constraint "test_pkey"
@@ -383,7 +380,6 @@ <title>Conflicts</title>
CONTEXT: processing remote data during "INSERT" for replication target
relation "public.test" in transaction 716 at 2021-09-29 15:52:45.165754+00
</screen>
- <para>
The transaction ID that contains the change violating the constraint can be
found from those outputs (transaction ID 716 in the above case). The
transaction
can be skipped by using <command>ALTER SUBSCRIPTION ... SKIP</command> on
the
@@ -401,9 +397,9 @@ <title>Conflicts</title>
that it doesn't conflict with incoming changes, or dropping the conflicting
constraint
or unique index, or writing a trigger on the subscriber to suppress or
redirect
conflicting incoming changes, or as a last resort, by skipping the whole
transaction.
- Skipping the whole transaction includes skipping changes that may not
violate
+ Skipping the whole transaction includes skipping changes that might not
violate
any constraint. This can easily make the subscriber inconsistent,
especially if
- a user specifies the wrong transaction ID or the position of origin.
+ a user specifies the wrong transaction ID or the wrong position of origin.
</para>
</sect1>
diff --git a/doc/src/sgml/ref/alter_subscription.sgml
b/doc/src/sgml/ref/alter_subscription.sgml
index 8b4568ddab..7e0eb55653 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -212,16 +212,16 @@ <title>Parameters</title>
<term><literal>SKIP ( <replaceable
class="parameter">skip_option</replaceable> = <replaceable
class="parameter">value</replaceable> [, ... ] )</literal></term>
<listitem>
<para>
- Skips applying all changes of the specified transaction. If incoming
data
- violates any constraints, the logical replication will stop until it is
+ Skips applying all changes of the specified remote transaction. If
incoming data
+ violates any constraints, logical replication will stop until it is
resolved. The resolution can be done either by changing data on the
subscriber so that it doesn't conflict with incoming changes or by
skipping
- the whole transaction. Using <command> ALTER SUBSCRIPTION ... SKIP
</command>
+ the whole transaction. Using the <command>ALTER SUBSCRIPTION ...
SKIP</command>
command, the logical replication worker skips all data modification
changes
- within the specified transaction including changes that may not violate
+ within the specified transaction, including changes that might not
violate
the constraint, so, it should only be used as a last resort. This option
has
no effect on the transactions that are already prepared by enabling
- <literal>two_phase</literal> on subscriber. After the logical
replication
+ <literal>two_phase</literal> on subscriber. After logical replication
successfully skips the transaction, the transaction ID (stored in
<structname>pg_subscription</structname>.<structfield>subskipxid</structfield>)
is cleared. See <xref linkend="logical-replication-conflicts"/> for
@@ -237,9 +237,9 @@ <title>Parameters</title>
<term><literal>xid</literal> (<type>xid</type>)</term>
<listitem>
<para>
- Specifies the ID of the transaction whose changes are to be skipped
- by the logical replication worker. We don't support skipping
- individual subtransactions. Setting <literal>NONE</literal>
+ Specifies the ID of the remote transaction whose changes are to be
skipped
+ by the logical replication worker. Skipping
+ individual subtransactions is not supported. Setting
<literal>NONE</literal>
resets the transaction ID.
</para>
</listitem>
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 0ff0e00f19..b8fb7130a6 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -273,7 +273,7 @@ parse_subscription_options(ParseState *pstate, List
*stmt_options,
if (!TransactionIdIsNormal(xid))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("invalid
transaction id: %s", xid_str)));
+ errmsg("invalid
transaction ID: %s", xid_str)));
}
opts->specified_opts |= SUBOPT_XID;
diff --git a/src/backend/replication/logical/worker.c
b/src/backend/replication/logical/worker.c
index 0264e30112..0b6d9a203a 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -814,9 +814,6 @@ apply_handle_begin(StringInfo s)
remote_final_lsn = begin_data.final_lsn;
- /*
- * Enable skipping all changes of this transaction if specified.
- */
maybe_start_skipping_changes(begin_data.xid);
in_remote_transaction = true;
@@ -871,9 +868,6 @@ apply_handle_begin_prepare(StringInfo s)
remote_final_lsn = begin_data.prepare_lsn;
- /*
- * Enable skipping all changes of this transaction if specified
- */
maybe_start_skipping_changes(begin_data.xid);
in_remote_transaction = true;
@@ -1429,7 +1423,6 @@ apply_spooled_messages(TransactionId xid, XLogRecPtr lsn)
remote_final_lsn = lsn;
- /* Enable skipping all changes of this transaction if specified */
maybe_start_skipping_changes(xid);
/*
diff --git a/src/test/regress/expected/subscription.out
b/src/test/regress/expected/subscription.out
index 892b6739bc..82733eed98 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -111,11 +111,11 @@ SELECT subname, subskipxid FROM pg_subscription WHERE
subname = 'regress_testsub
-- fail
ALTER SUBSCRIPTION regress_testsub SKIP (xid = 0);
-ERROR: invalid transaction id: 0
+ERROR: invalid transaction ID: 0
ALTER SUBSCRIPTION regress_testsub SKIP (xid = 1);
-ERROR: invalid transaction id: 1
+ERROR: invalid transaction ID: 1
ALTER SUBSCRIPTION regress_testsub SKIP (xid = 2);
-ERROR: invalid transaction id: 2
+ERROR: invalid transaction ID: 2
-- fail - must be superuser
SET SESSION AUTHORIZATION 'regress_subscription_user2';
ALTER SUBSCRIPTION regress_testsub SKIP (xid = 100);
diff --git a/src/test/subscription/t/028_skip_xact.pl
b/src/test/subscription/t/028_skip_xact.pl
index 4c107fc8f5..efe28c71af 100644
--- a/src/test/subscription/t/028_skip_xact.pl
+++ b/src/test/subscription/t/028_skip_xact.pl
@@ -8,8 +8,8 @@
use Test::More tests => 7;
# Test skipping the transaction. This function must be called after the caller
-# inserting data that conflict with the subscriber. After waiting for the
-# subscription worker stats are updated, we skip the transaction in question
+# has inserted data that conflicts with the subscriber. After waiting for the
+# subscription worker stats to be updated, we skip the transaction in question
# by ALTER SUBSCRIPTION ... SKIP. Then, check if logical replication can
continue
# working by inserting $nonconflict_data on the publisher.
sub test_skip_xact
@@ -17,6 +17,8 @@ sub test_skip_xact
my ($node_publisher, $node_subscriber, $subname, $relname,
$nonconflict_data,
$expected, $xid, $msg) = @_;
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
# Wait for worker error
$node_subscriber->poll_query_until(
'postgres',
@@ -83,7 +85,7 @@ sub test_skip_xact
]);
$node_subscriber->start;
-# Initial table setup on both publisher and subscriber. On subscriber we
+# Initial table setup on both publisher and subscriber. On the subscriber, we
# create the same tables but with primary keys. Also, insert some data that
# will conflict with the data replicated from publisher later.
$node_publisher->safe_psql(
--
2.34.1
