On Wed, Jan 12, 2022 at 11:10 PM vignesh C <vignes...@gmail.com> wrote: > > On Wed, Jan 12, 2022 at 11:32 AM Masahiko Sawada <sawada.m...@gmail.com> > wrote: > > > > On Wed, Jan 12, 2022 at 12:21 PM Amit Kapila <amit.kapil...@gmail.com> > > wrote: > > > > > > On Wed, Jan 12, 2022 at 5:49 AM Masahiko Sawada <sawada.m...@gmail.com> > > > wrote: > > > > > > > > On Tue, Jan 11, 2022 at 7:08 PM Amit Kapila <amit.kapil...@gmail.com> > > > > wrote: > > > > > > > > > > On Tue, Jan 11, 2022 at 1:51 PM Masahiko Sawada > > > > > <sawada.m...@gmail.com> wrote: > > > > > > > > > > > > On second thought, the same is true for other cases, for example, > > > > > > preparing the transaction and clearing skip_xid while handling a > > > > > > prepare message. That is, currently we don't clear skip_xid while > > > > > > handling a prepare message but do that while handling > > > > > > commit/rollback > > > > > > prepared message, in order to avoid the worst case. If we do both > > > > > > while handling a prepare message and the server crashes between > > > > > > them, > > > > > > it ends up that skip_xid is cleared and the transaction will be > > > > > > resent, which is identical to the worst-case above. > > > > > > > > > > > > > > > > How are you thinking to update the skip xid before prepare? If we do > > > > > it in the same transaction then the changes in the catalog will be > > > > > part of the prepared xact but won't be committed. Now, say if we do it > > > > > after prepare, then the situation won't be the same because after > > > > > restart the same xact won't appear again. > > > > > > > > I was thinking to commit the catalog change first in a separate > > > > transaction while not updating origin LSN and then prepare an empty > > > > transaction while updating origin LSN. > > > > > > > > > > But, won't it complicate the handling if in the future we try to > > > enhance this API such that it skips partial changes like skipping only > > > for particular relation(s) or particular operations as discussed > > > previously in this thread? > > > > Right. I was thinking that if we accept the situation that the user > > has to set skip_xid again in case of the server crashes, we might be > > able to accept also the situation that the user has to clear skip_xid > > in a case of the server crashes. But it seems the former is less > > problematic. > > > > I've attached an updated patch that incorporated all comments I got so far. > > Thanks for the updated patch, few comments:
Thank you for the comments!
> 1) Currently skip xid is not displayed in describe subscriptions, can
> we include it too:
> \dRs+ sub1
> List of subscriptions
> Name | Owner | Enabled | Publication | Binary | Streaming | Two
> phase commit | Synchronous commit | Conninfo
> ------+---------+---------+-------------+--------+-----------+------------------+--------------------+--------------------------------
> sub1 | vignesh | t | {pub1} | f | f | e
> | off | dbname=postgres host=localhost
> (1 row)
>
> 2) This import "use PostgreSQL::Test::Utils;" is not required:
> +# Tests for skipping logical replication transactions.
> +use strict;
> +use warnings;
> +use PostgreSQL::Test::Cluster;
> +use PostgreSQL::Test::Utils;
> +use Test::More tests => 6;
>
> 3) Some of the comments uses a punctuation mark and some of them does
> not use, Should we keep it consistent:
> + # Wait for worker error
> + $node_subscriber->poll_query_until(
> + 'postgres',
>
> + # Set skip xid
> + $node_subscriber->safe_psql(
> + 'postgres',
>
> +# Create publisher node.
> +my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
> +$node_publisher->init(allows_streaming => 'logical');
>
>
> +# Create subscriber node.
> +my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
>
> 4) Should this be changed:
> + * True if we are skipping all data modification changes (INSERT,
> UPDATE, etc.) of
> + * the specified transaction at MySubscription->skipxid. Once we
> start skipping
> + * changes, we don't stop it until the we skip all changes of the
> transaction even
> + * if pg_subscription is updated that and MySubscription->skipxid
> gets changed or
> to:
> + * True if we are skipping all data modification changes (INSERT,
> UPDATE, etc.) of
> + * the specified transaction at MySubscription->skipxid. Once we
> start skipping
> + * changes, we don't stop it until we skip all changes of the transaction
> even
> + * if pg_subscription is updated that and MySubscription->skipxid
> gets changed or
>
> In "stop it until the we skip all changes", here the is not required.
>
I agree with all the comments above. I've attached an updated patch.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
v4-0001-Add-ALTER-SUBSCRIPTION-.-SKIP-to-skip-the-transac.patch
Description: Binary data
