0001-fixup-Add-ALTER-SUBSCRIPTION-.-SKIP-to-skip-the-tran.patch

From 751be8216317f1d996c7b4f9f0e915adff805567 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@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