On Wednesday, March 2, 2022 12:01 AM Masahiko Sawada <[email protected]>
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
