Thread: RE: pg_copy_logical_replication_slot doesn't copy the failover property

On Monday, February 24, 2025 10:46 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

(The previous email was blocked, here is another attempt)

> 
> On Sat, Feb 22, 2025 at 11:26 AM Shlok Kyal <shlok.kyal.oss@gmail.com>
> wrote:
> >
> > Adding Amit to the thread.
> >
> 
> Thanks.
> 
> Looking at the original complaint:
> 
> > It says
> >                  * To avoid potential issues with the slot synchronization
> where the
> >                  * restart_lsn of a replication slot can go backward, we set
> the
> >                  * failover option to false here.  This situation occurs when
> a slot
> >                  * on the primary server is dropped and immediately
> replaced with a
> >                  * new slot of the same name, created by copying from
> another existing
> >                  * slot.  However, the slot synchronization will only observe
> the
> >                  * restart_lsn of the same slot going backward.
> 
> It would be better to update the comments as well to make the potential issues
> with slot synchronization clear or mention the reference of other place where
> we have comments related to this race condition. Also, I think it is better to
> write about two_phase in the comments as well as in docs unless already
> mentioned. If you agree with updating the comments as well, shall we redirect
> this to hackers?

(Redirect to -hackers)

> 
> > >
> > > IIUC the two_phase field is also not copied from the source slot. I
> > > think we can clarify in the doc that these two fields are not copied.
> > >
> 
> +1.

Here is the new V3 patch set which updated the comments to make the
issue clearer.

After thinking more on the two_phase option, I didn't find an issue that prevent
us from copying its option value. So, it’s more intuitive to me to just
copy its value instead of adding doc for it. The 0002 includes the same and I will
keep testing to ensure there are no other issues missed.

Best Regards,
Hou zj

On Mon, Feb 24, 2025 at 4:29 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Here is the new V3 patch set which updated the comments to make the
> issue clearer.
>

I pushed your first patch after minor modifications.

> After thinking more on the two_phase option, I didn't find an issue that prevent
> us from copying its option value.
>

Please see the comments atop ReplicationSlotCreate() and study the
commit 19890a064ebf53dedcefed0d8339ed3d449b06e6 to understand the
reasons as to why we initially enabled only at create time. It is
possible that the risk mentioned in the commit doesn't hold true for
copy_slot functionality but can you please analyze the same and let us
know your thoughts on the same?

> So, it’s more intuitive to me to just
> copy its value instead of adding doc for it. The 0002 includes the same and I will
> keep testing to ensure there are no other issues missed.
>

It is better to start a separate thread to discuss copying two_phase
property. Even, if it ends with just some comments and doc change, it
deserves an independent discussion.

--
With Regards,
Amit Kapila.