Re: Handle infinite recursion in logical replication setup

On Mon, Aug 22, 2022 at 9:19 AM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Thursday, August 18, 2022 11:13 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Aug 17, 2022 at 12:34 PM Peter Smith <smithpb2250(at)gmail(dot)com>
> > wrote:
> > >
> > > On Wed, Aug 17, 2022 at 4:33 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> > wrote:
> > > >
> > > > On Wed, Aug 17, 2022 at 8:48 AM houzj(dot)fnst(at)fujitsu(dot)com
> > > > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > > > >
> > > > > On Tuesday, August 2, 2022 8:00 PM Amit Kapila
> > <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > > > On Tue, Jul 26, 2022 at 9:07 AM Amit Kapila
> > <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > >
> > > > > Thanks for the summary.
> > > > >
> > > > > I think it's fine to make the user use the copy_data option more
> > > > > carefully to prevent duplicate copies by reporting an ERROR.
> > > > >
> > > > > But I also have similar concern with Sawada-san as it's possible
> > > > > for user to receive an ERROR in some unexpected cases.
> > > > >
> > > > > For example I want to build bi-directional setup between two nodes:
> > > > >
> > > > > Node A: TABLE test (has actual data) Node B: TABLE test (empty)
> > > > >
> > > > > Step 1:
> > > > > CREATE PUBLICATION on both Node A and B.
> > > > >
> > > > > Step 2:
> > > > > CREATE SUBSCRIPTION on Node A with (copy_data = on)
> > > > > -- this is fine as there is no data on Node B
> > > > >
> > > > > Step 3:
> > > > > CREATE SUBSCRIPTION on Node B with (copy_data = on)
> > > > > -- this should be fine as user needs to copy data from Node A to
> > > > > Node B,
> > > > > -- but we still report an error for this case.
> > > > >
> > > > > It looks a bit strict to report an ERROR in this case and it seems
> > > > > not easy to avoid this. So, personally, I think it might be better
> > > > > to document the correct steps to build the bi-directional
> > > > > replication and probably also docuemnt the steps to recover if
> > > > > user accidently did duplicate initial copy if not documented yet.
> > > > >
> > > > > In addition, we could also LOG some additional information about
> > > > > the ORIGIN and initial copy which might help user to analyze if needed.
> > > > >
> > > >
> > > > But why LOG instead of WARNING? I feel in this case there is a
> > > > chance of inconsistent data so a WARNING like "publication "pub1"
> > > > could have data from multiple origins" can be given when the user
> > > > has specified
> > > > options: "copy_data = on, origin = NONE" while creating a
> > > > subscription. We give a WARNING during subscription creation when
> > > > the corresponding publication doesn't exist, eg.
> > > >
> > > > postgres=# create subscription sub1 connection 'dbname = postgres'
> > > > publication pub1;
> > > > WARNING: publication "pub1" does not exist in the publisher
> > > >
> > > > Then, we can explain in docs how users can avoid data
> > > > inconsistencies while setting up replication.
> > > >
> > >
> > > I was wondering if this copy/origin case really should be a NOTICE.
> > >
> >
> > We usually give NOTICE for some sort of additional implicit information, e.g.,
> > when we create a slot during CREATE SUBSCRIPTION
> > command: "NOTICE: created replication slot "sub1" on publisher". IMO, this is
> > likely to be a problem of data inconsistency so I think here we can choose
> > between WARNING and LOG. I prefer WARNING but okay with LOG as well if
> > others feel so. I think we can change this later as well if required. We do have an
> > option to not do anything and just document it but I feel it is better to give user
> > some indication of problem here because not everyone reads each update of
> > documentation.
> >
> > Jonathan, Sawada-San, Hou-San, and others, what do you think is the best way
> > to move forward here?
>
> I think it's fine to throw a WARNING in this case given that there is a
> chance of inconsistent data.

Since there was no objections to change it to throw a warning, I have
made the changes for the same.

I have made one change for the documentation patch.
The current steps states that:
1. Create a publication on primary3.
2. Lock table t1 on primary2 and primary3 in EXCLUSIVE mode until the
setup is completed. There is no need to lock table t1 on primary1
because any data changes made will be synchronized while creating the
subscription with copy_data = true.
3. Create a subscription on primary1 to subscribe to primary3.
4. Create a subscription on primary2 to subscribe to primary3.
5. Create a subscription on primary3 to subscribe to primary1. Use
copy_data = true so that the existing table data is copied during
initial sync.
6. Create a subscription on primary3 to subscribe to primary2. Use
copy_data = false because the initial table data would have been
already copied in the previous step.
7. Now the replication setup between primaries primary1, primary2 and
primary3 is complete. Incremental changes made on any primary will be
replicated to the other two primaries.

We found a problem with the proposed steps. Here the problem is that
we lock table t1 on primary2/primary3 in step2, then we create a
subscription on primary3 in step5. While we create subscription in
step5, tablesync worker will try to lock the table in primary3 during
step5 before copying, as we have already taken a lock, this process
will wait.
Even if the lock is released in this step just before creating
subscription, there is a chance that some other transaction can
acquire the lock and change the data which will result in inconsistent
data. Here the locking solution cannot handle adding a new primary in
a consistent way.

I have changed the lock step to mention that user need to ensure no
data changes happen until the setup is completed.
The attached v42 patch has the changes for the same.

Regards,
Vignesh