RE: Skipping logical replication transactions on subscriber side

On Wednesday, March 2, 2022 12:01 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I've attached an updated patch along with two patches for cfbot tests since the
> main patch (0003) depends on the other two patches. Both
> 0001 and 0002 patches are the same ones I attached on another thread[2].
Hi, few comments on v12-0003-Add-ALTER-SUBSCRIPTION-.-SKIP-to-skip-the-transa.patch.


(1) doc/src/sgml/ref/alter_subscription.sgml


+    <term><literal>SKIP ( <replaceable class="parameter">skip_option</replaceable> = <replaceable class="parameter">value</r$
...
+      ...After logical replication
+      successfully skips the transaction or commits non-empty transaction,
+      the LSN (stored in
+      <structname>pg_subscription</structname>.<structfield>subskiplsn</structfield>)
+      is cleared.  See <xref linkend="logical-replication-conflicts"/> for
+      the details of logical replication conflicts.
+     </para>
...
+        <term><literal>lsn</literal> (<type>pg_lsn</type>)</term>
+        <listitem>
+         <para>
+          Specifies the commit LSN 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 LSN.


I think we'll extend the SKIP option choices in the future besides the 'lsn' option.
Then, one sentence "After logical replication successfully skips the transaction or commits non-empty
transaction, the LSN .. is cleared" should be moved to the explanation for 'lsn' section,
if we think this behavior to reset LSN is unique for 'lsn' option ?


(2) doc/src/sgml/catalogs.sgml

+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>subskiplsn</structfield> <type>pg_lsn</type>
+      </para>
+      <para>
+       Commit LSN of the transaction whose changes are to be skipped, if a valid
+       LSN; otherwise <literal>0/0</literal>.
+      </para></entry>
+     </row>
+

We need to cover the PREPARE that keeps causing errors on the subscriber.
This would apply to the entire patch (e.g. the rename of skip_xact_commit_lsn)

(3) apply_handle_commit_internal comments

 /*
  * Helper function for apply_handle_commit and apply_handle_stream_commit.
+ * Return true if the transaction was committed, otherwise return false.
  */

If we want to make the new added line alinged with other functions in worker.c,
we should insert one blank line before it ?


(4) apply_worker_post_transaction

I'm not sure if the current refactoring is good or not.
For example, the current HEAD calls pgstat_report_stat(false)
for a commit case if we are in a transaction in apply_handle_commit_internal.
On the other hand, your refactoring calls pgstat_report_stat unconditionally
for apply_handle_commit path. I'm not sure if there
are many cases to call apply_handle_commit without opening a transaction,
but is that acceptable ?

Also, the name is a bit broad.
How about making a function only for stopping and resetting LSN at this stage ?


(5) comments for clear_subscription_skip_lsn

How about changing the comment like below  ?

From:
Clear subskiplsn of pg_subscription catalog
To:
Clear subskiplsn of pg_subscription catalog with origin state update


Best Regards,
	Takamichi Osumi