Thread: pg_copy_logical_replication_slot doesn't copy the failover property
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.
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
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
Attachment
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
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