RE: Skipping logical replication transactions on subscriber side

On Wednesday, March 2, 2022 12:01 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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