Re: Skipping logical replication transactions on subscriber side

On Thu, Mar 10, 2022 at 2:10 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> 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.

Thank you for the comments.

>
>
> (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 ?

Hmm, I think that regardless of the type of option (e.g., relid, xid,
and action whatever), resetting the specified something after that is
specific to SKIP command. SKIP command should have an effect on only
the first non-empty transaction. Otherwise, we could end up leaving it
if the user mistakenly specifies the wrong one.

>
>
> (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)

Fixed.

>
> (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 ?

This part is removed.

>
>
> (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 ?

Agreed, it seems to be overkill. I'll revert that change.

>
>
> (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
>

Updated.

I'll submit an updated patch that incorporated comments I got so far
and is rebased to disable_on_error patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/