Обсуждение: Propose: Adding a '--enable-failover' option to 'pg_createsubscriber'

Поиск
Список
Период
Сортировка

Propose: Adding a '--enable-failover' option to 'pg_createsubscriber'

От
Ioseph Kim
Дата:
Hi

A failover option has been added to the CREATE SUBSCRITION command, but this functionality isn't easily accessible
usingthe pg_createsubscriber tool.
 

Subscriptions created using pg_createsubscriber must be configured for failover via an alter operation.

To address this issue, we've added a simple --enable-failover option to the pg_createsubscriber tool.

This patch is simple. It doesn't handle exceptions or provide any TAP test code.

Please review this and we hope the development team will refine it further.

ioseph

Вложения

Re:Propose: Adding a '--enable-failover' option to 'pg_createsubscriber'

От
高雪玉
Дата:
Hi, 
I have one comment to following comment:

pg_createsubscriber.c, the comment is not correct as this new option is not related with logical replication slot.
bool failover; /* enable failover option of logical replication slot */

Suggest to change to:
/* enable failover option of subscription */

Thanks,
Xueyu Gao 
HighGo Software Co., Ltd.
https://www.highgo.com/

At 2025-12-10 17:03:48, "Ioseph Kim" <pgsql-kr@postgresql.kr> wrote:
>Hi
>
>A failover option has been added to the CREATE SUBSCRITION command, but this functionality isn't easily accessible using the pg_createsubscriber tool.
>
>Subscriptions created using pg_createsubscriber must be configured for failover via an alter operation.
>
>To address this issue, we've added a simple --enable-failover option to the pg_createsubscriber tool.
>
>This patch is simple. It doesn't handle exceptions or provide any TAP test code.
>
>Please review this and we hope the development team will refine it further.
>
>ioseph

Re: Propose: Adding a '--enable-failover' option to 'pg_createsubscriber'

От
Chao Li
Дата:
> On Dec 10, 2025, at 17:03, Ioseph Kim <pgsql-kr@postgresql.kr> wrote:
>
> Hi
>
> A failover option has been added to the CREATE SUBSCRITION command, but this functionality isn't easily accessible
usingthe pg_createsubscriber tool. 
>
> Subscriptions created using pg_createsubscriber must be configured for failover via an alter operation.
>
> To address this issue, we've added a simple --enable-failover option to the pg_createsubscriber tool.
>
> This patch is simple. It doesn't handle exceptions or provide any TAP test code.
>
> Please review this and we hope the development team will refine it further.
>
> ioseph
> <0001-Adding-a-enable-failover-option-to-pg_createsubscrib.patch>

Hi, Ioseph,

Thanks for the patch. I consider adding the “(failover=true)” option is useful. A few small comments:

1
```
+      <para>
+       Enables <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+       commit for the subscription.
+       The default is <literal>false</literal>.
+      </para>
```

This doc change is confusing. What is “failover commit”? I guess you meant “failover option”?

I’d suggest a phrase like:

“Enables the subscription’s failover option, allowing its logical replication slot to be used after failover.”

2
```
+    printf(_("      --enable-failover           enable failover\n"));
```

If we look at the existing “—enable-two-phase” help text, we consider this help text is too simple. I’d suggest: enable
syncingreplication slots associated with the subscription. 

3
```
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -49,6 +49,7 @@ struct CreateSubscriberOptions
     int            recovery_timeout;    /* stop recovery after this time */
     bool        all_dbs;        /* all option */
     SimpleStringList objecttypes_to_clean;    /* list of object types to cleanup */
+    bool       failover;        /* enable failover option of subscription */
 };

 /* per-database publication/subscription info */
@@ -73,6 +74,7 @@ struct LogicalRepInfos
 {
     struct LogicalRepInfo *dbinfo;
     bool        two_phase;        /* enable-two-phase option */
+    bool        failover;        /* enable failover option of logical replication slot */
     bits32        objecttypes_to_clean;    /* flags indicating which object types
                                          * to clean up on subscriber */
 };
```

Add comments for “failover” in the two structures are inconsistent. The latter one is incorrect, the option is for
“createsubscription” command but for a slot. 

4 You need to run pgindent as I saw some format problems.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/







Re: Propose: Adding a '--enable-failover' option to 'pg_createsubscriber'

От
Ioseph Kim
Дата:
thanks for your comment.

There are two failover variables, one is CreateSubscriberOptions.failover, the other is LogicalRepInfos.failover.
That comment is for LogicalRepInfos.failover.
LogicalRepInfos.failover variable is used when logical replication slot will be created for the subscription.

ioseph

On Wed, Dec 10, 2025 at 06:11:46PM +0800, 高雪玉  wrote:
> Hi, 
> I have one comment to following comment:
> 
> 
> pg_createsubscriber.c, the comment is not correct as this new option is not related with logical replication slot.
> bool failover; /* enable failover option of logical replication slot */
> 
> 
> Suggest to change to:
> /* enable failover option of subscription */
> 
> 
> Thanks,
> Xueyu Gao 
> HighGo Software Co., Ltd.
> https://www.highgo.com/
> 
> 
> At 2025-12-10 17:03:48, "Ioseph Kim" <pgsql-kr@postgresql.kr> wrote:
> >Hi
> >
> >A failover option has been added to the CREATE SUBSCRITION command, but this functionality isn't easily accessible
usingthe pg_createsubscriber tool.
 
> >
> >Subscriptions created using pg_createsubscriber must be configured for failover via an alter operation.
> >
> >To address this issue, we've added a simple --enable-failover option to the pg_createsubscriber tool.
> >
> >This patch is simple. It doesn't handle exceptions or provide any TAP test code.
> >
> >Please review this and we hope the development team will refine it further.
> >
> >ioseph



Re: Propose: Adding a '--enable-failover' option to 'pg_createsubscriber'

От
Ioseph Kim
Дата:
Hi, Evan

thanks for your comments.

I modified like a attached patch file.
I used pgindent, thank you.

CreateSubscriberOptions.failover is used in CREATE SUBSCRIPTION,
but LogicalRepInfos.failover is used in pg_create_logical_replication_slot()
so I left these comments as is.

ioseph


On Thu, Dec 11, 2025 at 07:28:13AM +0800, Chao Li wrote:
> 
> > On Dec 10, 2025, at 17:03, Ioseph Kim <pgsql-kr@postgresql.kr> wrote:
> > 
> > Hi
> > 
> > A failover option has been added to the CREATE SUBSCRITION command, but this functionality isn't easily accessible
usingthe pg_createsubscriber tool.
 
> > 
> > Subscriptions created using pg_createsubscriber must be configured for failover via an alter operation.
> > 
> > To address this issue, we've added a simple --enable-failover option to the pg_createsubscriber tool.
> > 
> > This patch is simple. It doesn't handle exceptions or provide any TAP test code.
> > 
> > Please review this and we hope the development team will refine it further.
> > 
> > ioseph
> > <0001-Adding-a-enable-failover-option-to-pg_createsubscrib.patch>
> 
> Hi, Ioseph,
> 
> Thanks for the patch. I consider adding the “(failover=true)” option is useful. A few small comments:
> 
> 1
> ```
> +      <para>
> +       Enables <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
> +       commit for the subscription.
> +       The default is <literal>false</literal>.
> +      </para>
> ```
> 
> This doc change is confusing. What is “failover commit”? I guess you meant “failover option”?
> 
> I’d suggest a phrase like:
> 
> “Enables the subscription’s failover option, allowing its logical replication slot to be used after failover.”
> 
> 2
> ```
> +    printf(_("      --enable-failover           enable failover\n"));
> ```
> 
> If we look at the existing “—enable-two-phase” help text, we consider this help text is too simple. I’d suggest:
enablesyncing replication slots associated with the subscription.
 
> 
> 3
> ```
> --- a/src/bin/pg_basebackup/pg_createsubscriber.c
> +++ b/src/bin/pg_basebackup/pg_createsubscriber.c
> @@ -49,6 +49,7 @@ struct CreateSubscriberOptions
>      int            recovery_timeout;    /* stop recovery after this time */
>      bool        all_dbs;        /* all option */
>      SimpleStringList objecttypes_to_clean;    /* list of object types to cleanup */
> +    bool       failover;        /* enable failover option of subscription */
>  };
>  
>  /* per-database publication/subscription info */
> @@ -73,6 +74,7 @@ struct LogicalRepInfos
>  {
>      struct LogicalRepInfo *dbinfo;
>      bool        two_phase;        /* enable-two-phase option */
> +    bool        failover;        /* enable failover option of logical replication slot */
>      bits32        objecttypes_to_clean;    /* flags indicating which object types
>                                           * to clean up on subscriber */
>  };
> ```
> 
> Add comments for “failover” in the two structures are inconsistent. The latter one is incorrect, the option is for
“createsubscription” command but for a slot.
 
> 
> 4 You need to run pgindent as I saw some format problems.
> 
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
> 
> 
> 
>

Вложения

Re: Propose: Adding a '--enable-failover' option to 'pg_createsubscriber' ver.3

От
Ioseph Kim
Дата:
Hi

TAP test codes are added in the patch
and manual has become more user-friendly.

Please review the patch and let me know if you see a better solution. I’ll be glad to update it accordingly.

ioseph

On Wed, Dec 10, 2025 at 06:03:48PM +0900, Ioseph Kim wrote:
> Hi
> 
> A failover option has been added to the CREATE SUBSCRITION command, but this functionality isn't easily accessible
usingthe pg_createsubscriber tool.
 
> 
> Subscriptions created using pg_createsubscriber must be configured for failover via an alter operation.
> 
> To address this issue, we've added a simple --enable-failover option to the pg_createsubscriber tool.
> 
> This patch is simple. It doesn't handle exceptions or provide any TAP test code.
> 
> Please review this and we hope the development team will refine it further.
> 
> ioseph

Вложения

Re:Re: Propose: Adding a '--enable-failover' option to 'pg_createsubscriber' ver.3

От
"Xueyu Gao"
Дата:

At 2025-12-11 14:39:16, "Ioseph Kim" <pgsql-kr@postgresql.kr> wrote:
>Hi
>
>TAP test codes are added in the patch
>and manual has become more user-friendly.
>
>Please review the patch and let me know if you see a better solution. I’ll be glad to update it accordingly.
>
>ioseph
>
>On Wed, Dec 10, 2025 at 06:03:48PM +0900, Ioseph Kim wrote:
>> Hi
>> 
>> A failover option has been added to the CREATE SUBSCRITION command, but this functionality isn't easily accessible using the pg_createsubscriber tool.
>> 
>> Subscriptions created using pg_createsubscriber must be configured for failover via an alter operation.
>> 
>> To address this issue, we've added a simple --enable-failover option to the pg_createsubscriber tool.
>> 
>> This patch is simple. It doesn't handle exceptions or provide any TAP test code.
>> 
>> Please review this and we hope the development team will refine it further.
>> 
>> ioseph

Hi, ioseph,
I took a look at the doc part and have two comments.
/doc/src/sgml/ref/pg_createsubscriber.sgml
1. "The default is <literal>false</literal>" , it'd be better to add "value" after the "default".
2. I think the following content should be surrounded by <para> and </para>, so the <para> should be moved to be in front of "When this option is enabled..."
+ When this option is enabled, the connection string used in the <option>--publisher-server</option>
+ option may be adjusted to support failover. For example, by specifying multiple hosts
+ and using <literal>target_session_attrs=read-write</literal>.
+ <para>
+ </para>
+ </listitem>
+ </varlistentry>
Thanks,
Xueyu Gao

Re:Re:Re: Propose: Adding a '--enable-failover' option to 'pg_createsubscriber' ver.3

От
"Xueyu Gao"
Дата:
At 2025-12-11 15:13:40, "Xueyu Gao" <gaoxueyu_hope@163.com> wrote:

At 2025-12-11 14:39:16, "Ioseph Kim" <pgsql-kr@postgresql.kr> wrote:
>Hi
>
>TAP test codes are added in the patch
>and manual has become more user-friendly.
>
>Please review the patch and let me know if you see a better solution. I’ll be glad to update it accordingly.
>
>ioseph
>
>On Wed, Dec 10, 2025 at 06:03:48PM +0900, Ioseph Kim wrote:
>> Hi
>> 
>> A failover option has been added to the CREATE SUBSCRITION command, but this functionality isn't easily accessible using the pg_createsubscriber tool.
>> 
>> Subscriptions created using pg_createsubscriber must be configured for failover via an alter operation.
>> 
>> To address this issue, we've added a simple --enable-failover option to the pg_createsubscriber tool.
>> 
>> This patch is simple. It doesn't handle exceptions or provide any TAP test code.
>> 
>> Please review this and we hope the development team will refine it further.
>> 
>> ioseph

Hi, ioseph,
I took a look at the doc part and have two comments.
/doc/src/sgml/ref/pg_createsubscriber.sgml
1. "The default is <literal>false</literal>" , it'd be better to add "value" after the "default".
2. I think the following content should be surrounded by <para> and </para>, so the <para> should be moved to be in front of "When this option is enabled..."
+ When this option is enabled, the connection string used in the <option>--publisher-server</option>
+ option may be adjusted to support failover. For example, by specifying multiple hosts
+ and using <literal>target_session_attrs=read-write</literal>.
+ <para>
+ </para>
+ </listitem>
+ </varlistentry>
Thanks,
Xueyu Gao

One more comment:
src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+ 'replication slot are created with the failover option enabled');
This sentence should be "replication slots are ..."
Thanks,
Xueyu Gao

Re: Re: Propose: Adding a '--enable-failover' option to 'pg_createsubscriber' ver.3

От
Ioseph Kim
Дата:
Thanks Xueyu

1.
In this manual, The phrase "The default is ..." is used consistently.
so, It would be better not to change it to maintain consistency.

2.
I made a mistake.
If this gets committed,
a contributer would fix the <para> line to be placed at the beginning of the paragraph, please.

ioseph

On Thu, Dec 11, 2025 at 03:13:40PM +0800, Xueyu Gao wrote:
> 
> 
> At 2025-12-11 14:39:16, "Ioseph Kim" <pgsql-kr@postgresql.kr> wrote:
> >Hi
> >
> >TAP test codes are added in the patch
> >and manual has become more user-friendly.
> >
> >Please review the patch and let me know if you see a better solution. I’ll be glad to update it accordingly.
> >
> >ioseph
> >
> >On Wed, Dec 10, 2025 at 06:03:48PM +0900, Ioseph Kim wrote:
> >> Hi
> >> 
> >> A failover option has been added to the CREATE SUBSCRITION command, but this functionality isn't easily accessible
usingthe pg_createsubscriber tool.
 
> >> 
> >> Subscriptions created using pg_createsubscriber must be configured for failover via an alter operation.
> >> 
> >> To address this issue, we've added a simple --enable-failover option to the pg_createsubscriber tool.
> >> 
> >> This patch is simple. It doesn't handle exceptions or provide any TAP test code.
> >> 
> >> Please review this and we hope the development team will refine it further.
> >> 
> 
> >> ioseph
> 
> 
> Hi, ioseph, 
> 
> I took a look at the doc part and have two comments.
> 
> /doc/src/sgml/ref/pg_createsubscriber.sgml
> 
> 1. "The default is <literal>false</literal>" , it'd be better to add "value" after the "default". 
> 
> 2. I think the following content should be surrounded by <para> and </para>, so the <para> should be moved to be in
frontof "When this option is enabled..."
 
> 
> +       When this option is enabled, the connection string used in the <option>--publisher-server</option>
> 
> +       option may be adjusted to support failover. For example, by specifying multiple hosts 
> 
> +       and using <literal>target_session_attrs=read-write</literal>.
> 
> +      <para>
> 
> +      </para>
> 
> +     </listitem>
> 
> +    </varlistentry>
> Thanks,
> Xueyu Gao