Re: Handle infinite recursion in logical replication setup

On Fri, Jul 22, 2022 at 1:39 AM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>
> Hi,
>
> On 7/21/22 6:34 AM, vignesh C wrote:
> > On Thu, Jul 21, 2022 at 2:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >>
> >> On Wed, Jul 20, 2022 at 2:33 PM vignesh C <vignesh21@gmail.com> wrote:
> >>>
> >>> Modified. Apart from this I have run pgperltidy on the perl file and
> >>> renamed 032_origin.pl to 030_origin.pl as currently there is
> >>> 029_on_error.pl, 031_column_list.pl and there is no 030_*****.pl file.
> >>> Thanks for the comment, the attached patch has the changes for the same.
> >>>
> >>
> >> Pushed. Kindly rebase the remaining patches.
> >
> > Thanks for pushing the patch.
> > The attached v37 version contains the rebased patch for the remaining patches.
>
> Thanks for the work on this feature -- this is definitely very helpful
> towards supporting more types of use cases with logical replication!
>
> I've read through the proposed documentation and did some light testing
> of the patch. I have two general comments about the docs as they
> currently read:
>
> 1. I'm concerned by calling this "Bidirectional replication" in the docs
> that we are overstating the current capabilities. I think this is
> accentuated int he opening paragraph:
>
> ==snip==
>   Bidirectional replication is useful for creating a multi-master database
>   environment for replicating read/write operations performed by any of the
>   member nodes.
> ==snip==
>
> For one, we're not replicating reads, we're replicating writes. Amongst
> the writes, at this point we're only replicating DML. A reader could
> think that deploying can work for a full bidirectional solution.

I have changed read/write operations to write operations. I have also
added a note saying "The logical replication restrictions applies to
the replication between primaries also.", to clarify that non DML
operations and other restrictions apply in this case too.

> (Even if we're aspirationally calling this section "Bidirectional
> replication", that does make it sound like we're limited to two nodes,
> when we can support more than two).
>
> Perhaps "Logical replication between writers" or "Logical replication
> between primaries" or "Replicating changes between primaries", or
> something better.

I have changed it to "Replication between primaries".

> 2. There is no mention of conflicts in the documentation, e.g.
> referencing the "Conflicts" section of the documentation. It's very easy
> to create a conflicting transaction that causes a subscriber to be
> unable to continue to apply transactions:
>
>    -- DB 1
>    CREATE TABLE abc (id int);
>    CREATE PUBLICATION node1 FOR ALL TABLES ;
>
>    -- DB2
>    CREATE TABLE abc (id int);
>    CREATE PUBLICATION node2 FOR ALL TABLES ;
>    CREATE SUBSCRIPTION node2_node1
>      CONNECTION 'dbname=logi port=5433'
>      PUBLICATION node1
>      WITH (copy_data = off, origin = none);
>
>    -- DB1
>    CREATE SUBSCRIPTION node1_node2
>      CONNECTION 'dbname=logi port=5434'
>      PUBLICATION node2
>      WITH (copy_data = off, origin = none);
>    INSERT INTO abc VALUES (1);
>
>    -- DB2
>    INSERT INTO abc VALUES (2);
>
>    -- DB1
>    ALTER TABLE abc ADD PRIMARY KEY id;
>    INSERT INTO abc VALUES (3);
>
>    -- DB2
>    INSERT INTO abc VALUES (3);
>
>    -- DB1 cannot apply the transactions
>
> At a minimum, I think we should reference the documentation we have in
> the logical replication section on conflicts. We may also want to advise
> that a user is responsible for designing their schemas in a way to
> minimize the risk of conflicts.

Added a  note for the same and referred it to the conflicts section.

Thanks for the comments, the attached v38 patch has the changes for the same.

Regards,
Vignesh