Horton

Thread

Commits

  1. Doc: Fix pg_copy_logical_replication_slot description.

  2. Add option to enable two_phase commits via pg_create_logical_replication_slot.

  1. pg_copy_logical_replication_slot doesn't copy the failover property

    The Post Office <noreply@postgresql.org> — 2025-02-17T05:01:48Z

    The following documentation comment has been logged on the website:

Page: https://www.postgresql.org/docs/17/functions-admin.html
Description:

The documentation for pg_copy_logical_replication_slot doesn't mention that
the failover property for the logical slot is not copied.

I assumed there was a good reason for this, and I found a comment in the
source code that explains it (although I don't really understand).
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.

I assumed that by default, all properties from the original slot would be
copied, so this function left me wondering why my logical replication slots
were not being synced to the replica.

  2. Re: pg_copy_logical_replication_slot doesn't copy the failover property

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-02-19T07:10:34Z

    On Tue, 18 Feb 2025 at 19:49, PG Doc comments form
<noreply@postgresql.org> wrote:
>
> The following documentation comment has been logged on the website:
>
> Page: https://www.postgresql.org/docs/17/functions-admin.html
> Description:
>
> The documentation for pg_copy_logical_replication_slot doesn't mention that
> the failover property for the logical slot is not copied.
>
> I assumed there was a good reason for this, and I found a comment in the
> source code that explains it (although I don't really understand).
> 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.
>
> I assumed that by default, all properties from the original slot would be
> copied, so this function left me wondering why my logical replication slots
> were not being synced to the replica.

Hi,

The failover option is set to false by default while copying of the
slots as it may cause some issue during slot synchronization as per
discussion [1].

I have created a patch to update the documentation for the same.

[1]: https://www.postgresql.org/message-id/CAD21AoCoX%2Bjhy_i3v%2BT2s78NG_0HH1oXOUiTOWhDdxVPBtDHKA%40mail.gmail.com

Thanks and Regards,
Shlok Kyal

  3. Re: pg_copy_logical_replication_slot doesn't copy the failover property

    Nisha Moond <nisha.moond412@gmail.com> — 2025-02-20T04:09:27Z

    On Wed, Feb 19, 2025 at 12:40 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> Hi,
>
> The failover option is set to false by default while copying of the
> slots as it may cause some issue during slot synchronization as per
> discussion [1].
>
> I have created a patch to update the documentation for the same.
>

Thanks for the patch.
A couple of comments regarding the sentence:

+        not copied and set as <literal>false</literal> by default due to
+        potential issues with the slot synchronization.

1) / and set as/ and is set to
"set to" is more natural and clearer when assigning a specific value
compared to "set as."

2) "due to potential issues..." wording implies that it is addressing
existing issues. I feel it would be better to say:
"... <literal>false</literal> by default to avoid potential issues
with the slot synchronization."

--
Thanks,
Nisha

  4. Re: pg_copy_logical_replication_slot doesn't copy the failover property

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-02-20T04:24:52Z

    On Thu, 20 Feb 2025 at 09:39, Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> On Wed, Feb 19, 2025 at 12:40 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > Hi,
> >
> > The failover option is set to false by default while copying of the
> > slots as it may cause some issue during slot synchronization as per
> > discussion [1].
> >
> > I have created a patch to update the documentation for the same.
> >
>
> Thanks for the patch.
> A couple of comments regarding the sentence:
>
> +        not copied and set as <literal>false</literal> by default due to
> +        potential issues with the slot synchronization.
>
> 1) / and set as/ and is set to
> "set to" is more natural and clearer when assigning a specific value
> compared to "set as."
>
> 2) "due to potential issues..." wording implies that it is addressing
> existing issues. I feel it would be better to say:
> "... <literal>false</literal> by default to avoid potential issues
> with the slot synchronization."
>

Hi Nisha,

I have addressed the comments and attached the updated v2 patch.

Thanks and Regards,
Shlok Kyal

  5. Re: pg_copy_logical_replication_slot doesn't copy the failover property

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-02-20T17:48:59Z

    On Wed, Feb 19, 2025 at 8:25 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Thu, 20 Feb 2025 at 09:39, Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > On Wed, Feb 19, 2025 at 12:40 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > > Hi,
> > >
> > > The failover option is set to false by default while copying of the
> > > slots as it may cause some issue during slot synchronization as per
> > > discussion [1].
> > >
> > > I have created a patch to update the documentation for the same.
> > >
> >
> > Thanks for the patch.
> > A couple of comments regarding the sentence:
> >
> > +        not copied and set as <literal>false</literal> by default due to
> > +        potential issues with the slot synchronization.
> >
> > 1) / and set as/ and is set to
> > "set to" is more natural and clearer when assigning a specific value
> > compared to "set as."
> >
> > 2) "due to potential issues..." wording implies that it is addressing
> > existing issues. I feel it would be better to say:
> > "... <literal>false</literal> by default to avoid potential issues
> > with the slot synchronization."
> >
>
> Hi Nisha,
>
> I have addressed the comments and attached the updated v2 patch.

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.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

  6. Re: pg_copy_logical_replication_slot doesn't copy the failover property

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-02-22T05:56:31Z

    Adding Amit to the thread.

On Thu, 20 Feb 2025 at 23:19, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Feb 19, 2025 at 8:25 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Thu, 20 Feb 2025 at 09:39, Nisha Moond <nisha.moond412@gmail.com> wrote:
> > >
> > > On Wed, Feb 19, 2025 at 12:40 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > > > Hi,
> > > >
> > > > The failover option is set to false by default while copying of the
> > > > slots as it may cause some issue during slot synchronization as per
> > > > discussion [1].
> > > >
> > > > I have created a patch to update the documentation for the same.
> > > >
> > >
> > > Thanks for the patch.
> > > A couple of comments regarding the sentence:
> > >
> > > +        not copied and set as <literal>false</literal> by default due to
> > > +        potential issues with the slot synchronization.
> > >
> > > 1) / and set as/ and is set to
> > > "set to" is more natural and clearer when assigning a specific value
> > > compared to "set as."
> > >
> > > 2) "due to potential issues..." wording implies that it is addressing
> > > existing issues. I feel it would be better to say:
> > > "... <literal>false</literal> by default to avoid potential issues
> > > with the slot synchronization."
> > >
> >
> > Hi Nisha,
> >
> > I have addressed the comments and attached the updated v2 patch.
>
> 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.
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com

  7. Re: pg_copy_logical_replication_slot doesn't copy the failover property

    Amit Kapila <amit.kapila16@gmail.com> — 2025-02-24T02:46:20Z

    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?

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

-- 
With Regards,
Amit Kapila.

  8. RE: pg_copy_logical_replication_slot doesn't copy the failover property

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2025-02-24T10:58:52Z

    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

  9. Re: pg_copy_logical_replication_slot doesn't copy the failover property

    Amit Kapila <amit.kapila16@gmail.com> — 2025-02-25T05:31:56Z

    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.