pg_recvlogical cannot create slots with failover=true

Hi,

On Tue, Apr 01, 2025 at 08:01:32AM +0000, Hayato Kuroda (Fujitsu) wrote:

Attached patch implements it. Since -f/-F option has already been used, -O was
chosen for the short-version. Better idea is very welcomed.

Maybe we don't need a short option at all for this, at least initially?

Michael

Dear Michael,

Maybe we don't need a short option at all for this, at least initially?

Indeed, updated.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Wed, Apr 2, 2025 at 11:57 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Michael,

Maybe we don't need a short option at all for this, at least initially?

Indeed, updated.

Thank you for updating the patch. Here are some minor comments:

         The <option>--two-phase</option> can be specified with
         <option>--create-slot</option> to enable decoding of prepared
transactions.
+        Also, <option>--falover</option> can be specified with
+        <option>--create-slot</option> to create replication slot which can be
+        synchronized to the standby.

How about rewording the whole sentence like:

+        The <option>--two-phase</option> and
<option>--failover</option> options
+        can be specified with <option>--create-slot</option>.

Explaining both options here seems redundant to me.

---
+       <para>
+        Allows created replication slot to be synchronized to the standbys.
+        This option may only be specified with <option>--create-slot</option>.
+       </para>

How about slightly rewording to like:

+        Enables the slot to be synchronized to the standbys. This
option may only be
+        specified with <option>--create-slot</option>.

Also, the descriptions of pg_recvlogical options are written in
alphabetical order. Please put the description for --failover option
after -E/--endpos.

The rest looks good to me.

Regards,

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

Dear Sawada-san,

Thank you for updating the patch. Here are some minor comments:

The <option>--two-phase</option> can be specified with
<option>--create-slot</option> to enable decoding of prepared
transactions.
+        Also, <option>--falover</option> can be specified with
+        <option>--create-slot</option> to create replication slot which can
be
+        synchronized to the standby.

How about rewording the whole sentence like:

+        The <option>--two-phase</option> and
<option>--failover</option> options
+        can be specified with <option>--create-slot</option>.

Explaining both options here seems redundant to me.

Fixed.

---
+       <para>
+        Allows created replication slot to be synchronized to the standbys.
+        This option may only be specified with
<option>--create-slot</option>.
+       </para>

How about slightly rewording to like:

+        Enables the slot to be synchronized to the standbys. This
option may only be
+        specified with <option>--create-slot</option>.

Fixed. The description in usage() is adjusted based on this.

Also, the descriptions of pg_recvlogical options are written in
alphabetical order. Please put the description for --failover option
after -E/--endpos.

Right. I put because it had short-term '-o' in old version, but it was removed.
Fixed.

PSA new version.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, Apr 3, 2025 at 8:28 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Sawada-san,

Thank you for updating the patch. Here are some minor comments:

The <option>--two-phase</option> can be specified with
<option>--create-slot</option> to enable decoding of prepared
transactions.
+        Also, <option>--falover</option> can be specified with
+        <option>--create-slot</option> to create replication slot which can
be
+        synchronized to the standby.

How about rewording the whole sentence like:

+        The <option>--two-phase</option> and
<option>--failover</option> options
+        can be specified with <option>--create-slot</option>.

Explaining both options here seems redundant to me.

Fixed.

---
+       <para>
+        Allows created replication slot to be synchronized to the standbys.
+        This option may only be specified with
<option>--create-slot</option>.
+       </para>

How about slightly rewording to like:

+        Enables the slot to be synchronized to the standbys. This
option may only be
+        specified with <option>--create-slot</option>.

Fixed. The description in usage() is adjusted based on this.

Also, the descriptions of pg_recvlogical options are written in
alphabetical order. Please put the description for --failover option
after -E/--endpos.

Right. I put because it had short-term '-o' in old version, but it was removed.
Fixed.

Thank you for updating the patch! Pushed with small cosmetic changes.

Regards,

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

On Sat, Apr 5, 2025 at 1:45 AM Masahiko Sawada wrote:

Hi,

Thank you for updating the patch! Pushed with small cosmetic changes.

Thanks for pushing the feature !

I noticed one typo in the doc and here is a tiny patch to fix it.

-        The <option>--two-phase</option> and <option>--falover</option> options
+        The <option>--two-phase</option> and <option>--failover</option> options

Best Regards,
Hou zj

On Sun, Apr 6, 2025 at 7:19 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Sat, Apr 5, 2025 at 1:45 AM Masahiko Sawada wrote:

Hi,

Thank you for updating the patch! Pushed with small cosmetic changes.

Thanks for pushing the feature !

I noticed one typo in the doc and here is a tiny patch to fix it.

-        The <option>--two-phase</option> and <option>--falover</option> options
+        The <option>--two-phase</option> and <option>--failover</option> options

Thank you for the report and the patch! Pushed.

Regards,

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

On 07.04.25 21:15, Masahiko Sawada wrote:

On Sun, Apr 6, 2025 at 7:19 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Sat, Apr 5, 2025 at 1:45 AM Masahiko Sawada wrote:

Hi,

Thank you for updating the patch! Pushed with small cosmetic changes.

Thanks for pushing the feature !

I noticed one typo in the doc and here is a tiny patch to fix it.

-        The <option>--two-phase</option> and <option>--falover</option> options
+        The <option>--two-phase</option> and <option>--failover</option> options

Thank you for the report and the patch! Pushed.

I wonder if the option name --failover is ideal. To me, it sounds like
an action "do a failover!". Also consider that pg_recvlogical has other
action options such as --start and --create-slot, so it sounds a bit
like those.

Maybe something like --enable-failover would be better?

On Thu, Jun 12, 2025 at 10:42 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 07.04.25 21:15, Masahiko Sawada wrote:

On Sun, Apr 6, 2025 at 7:19 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Sat, Apr 5, 2025 at 1:45 AM Masahiko Sawada wrote:

Hi,

Thank you for updating the patch! Pushed with small cosmetic changes.

Thanks for pushing the feature !

I noticed one typo in the doc and here is a tiny patch to fix it.

-        The <option>--two-phase</option> and <option>--falover</option> options
+        The <option>--two-phase</option> and <option>--failover</option> options

Thank you for the report and the patch! Pushed.

I wonder if the option name --failover is ideal. To me, it sounds like
an action "do a failover!". Also consider that pg_recvlogical has other
action options such as --start and --create-slot, so it sounds a bit
like those.

Fair point.

Maybe something like --enable-failover would be better?

Sounds better, but probably the --two-phase option has the same issue?

Regards,

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

On Sat, Jun 14, 2025 at 3:11 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jun 12, 2025 at 10:42 PM Peter Eisentraut <peter@eisentraut.org> wrote:

-        The <option>--two-phase</option> and <option>--falover</option> options
+        The <option>--two-phase</option> and <option>--failover</option> options

Thank you for the report and the patch! Pushed.

I wonder if the option name --failover is ideal. To me, it sounds like
an action "do a failover!". Also consider that pg_recvlogical has other
action options such as --start and --create-slot, so it sounds a bit
like those.

Fair point.

Maybe something like --enable-failover would be better?

Sounds better, but probably the --two-phase option has the same issue?

Ideally, we should change both to maintain consistency across various
slot options. OTOH, as we have already described these options as: "
The --two-phase and --failover options can be specified with
--create-slot.", it is clear that these are slot options. The previous
version docs have a description: "The --two-phase can be specified
with --create-slot to enable decoding of prepared transactions." which
makes it even more clear that the two-phase is a slot option. The
options are named similarly in pg_create_logical_replication_slot API
and during CREATE SUBSCRIPTION, so, to some level, there is a
consistency in naming of these options across all APIs/tools. But,
their usage in a tool like pg_recvlogical could be perceived
differently as well, so it is also okay to consider naming them
differently.

--
With Regards,
Amit Kapila.

Dear Amit, Sawada, Peter,

I wonder if the option name --failover is ideal. To me, it sounds like
an action "do a failover!". Also consider that pg_recvlogical has other
action options such as --start and --create-slot, so it sounds a bit
like those.

Fair point.

Maybe something like --enable-failover would be better?

Sounds better, but probably the --two-phase option has the same issue?

Ideally, we should change both to maintain consistency across various
slot options. OTOH, as we have already described these options as: "
The --two-phase and --failover options can be specified with
--create-slot.", it is clear that these are slot options. The previous
version docs have a description: "The --two-phase can be specified
with --create-slot to enable decoding of prepared transactions." which
makes it even more clear that the two-phase is a slot option. The
options are named similarly in pg_create_logical_replication_slot API
and during CREATE SUBSCRIPTION, so, to some level, there is a
consistency in naming of these options across all APIs/tools. But,
their usage in a tool like pg_recvlogical could be perceived
differently as well, so it is also okay to consider naming them
differently.

Either name is fine for me, but I have a concern for the description. Now the
documentation says:

```
-t
--two-phase
Enables decoding of prepared transactions. This option may only be specified with --create-slot.
```

If we clarify the option is aimed for the slot, should we follow the
description in the protocol.sgml? I.e.,

```
-t
--two-phase
the slot supports decoding of two-phase commit. This option may only be specified with --create-slot.
```

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On 14.06.25 07:26, Amit Kapila wrote:

On Sat, Jun 14, 2025 at 3:11 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jun 12, 2025 at 10:42 PM Peter Eisentraut <peter@eisentraut.org> wrote:

-        The <option>--two-phase</option> and <option>--falover</option> options
+        The <option>--two-phase</option> and <option>--failover</option> options

Thank you for the report and the patch! Pushed.

I wonder if the option name --failover is ideal. To me, it sounds like
an action "do a failover!". Also consider that pg_recvlogical has other
action options such as --start and --create-slot, so it sounds a bit
like those.

Fair point.

Maybe something like --enable-failover would be better?

Sounds better, but probably the --two-phase option has the same issue?

Ideally, we should change both to maintain consistency across various
slot options. OTOH, as we have already described these options as: "
The --two-phase and --failover options can be specified with
--create-slot.", it is clear that these are slot options. The previous
version docs have a description: "The --two-phase can be specified
with --create-slot to enable decoding of prepared transactions." which
makes it even more clear that the two-phase is a slot option. The
options are named similarly in pg_create_logical_replication_slot API
and during CREATE SUBSCRIPTION, so, to some level, there is a
consistency in naming of these options across all APIs/tools. But,
their usage in a tool like pg_recvlogical could be perceived
differently as well, so it is also okay to consider naming them
differently.

Also note that we have a new pg_createsubscriber --enable-two-phase.

It would be nice if there was more consistency between similar/related
tools.

On Tue, Jun 17, 2025 at 12:59 AM Peter Eisentraut <peter@eisentraut.org> wrote:

On 14.06.25 07:26, Amit Kapila wrote:

On Sat, Jun 14, 2025 at 3:11 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jun 12, 2025 at 10:42 PM Peter Eisentraut <peter@eisentraut.org> wrote:

-        The <option>--two-phase</option> and <option>--falover</option> options
+        The <option>--two-phase</option> and <option>--failover</option> options

Thank you for the report and the patch! Pushed.

I wonder if the option name --failover is ideal. To me, it sounds like
an action "do a failover!". Also consider that pg_recvlogical has other
action options such as --start and --create-slot, so it sounds a bit
like those.

Fair point.

Maybe something like --enable-failover would be better?

Sounds better, but probably the --two-phase option has the same issue?

Ideally, we should change both to maintain consistency across various
slot options. OTOH, as we have already described these options as: "
The --two-phase and --failover options can be specified with
--create-slot.", it is clear that these are slot options. The previous
version docs have a description: "The --two-phase can be specified
with --create-slot to enable decoding of prepared transactions." which
makes it even more clear that the two-phase is a slot option. The
options are named similarly in pg_create_logical_replication_slot API
and during CREATE SUBSCRIPTION, so, to some level, there is a
consistency in naming of these options across all APIs/tools. But,
their usage in a tool like pg_recvlogical could be perceived
differently as well, so it is also okay to consider naming them
differently.

Also note that we have a new pg_createsubscriber --enable-two-phase.

Yeah, I also noticed the precedent.

It would be nice if there was more consistency between similar/related
tools.

I've attached the patch. Feedback is very welcome.

Regards,

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

On 17.06.25 20:19, Masahiko Sawada wrote:

Ideally, we should change both to maintain consistency across various
slot options. OTOH, as we have already described these options as: "
The --two-phase and --failover options can be specified with
--create-slot.", it is clear that these are slot options. The previous
version docs have a description: "The --two-phase can be specified
with --create-slot to enable decoding of prepared transactions." which
makes it even more clear that the two-phase is a slot option. The
options are named similarly in pg_create_logical_replication_slot API
and during CREATE SUBSCRIPTION, so, to some level, there is a
consistency in naming of these options across all APIs/tools. But,
their usage in a tool like pg_recvlogical could be perceived
differently as well, so it is also okay to consider naming them
differently.

Also note that we have a new pg_createsubscriber --enable-two-phase.

Yeah, I also noticed the precedent.

It would be nice if there was more consistency between similar/related
tools.

I've attached the patch. Feedback is very welcome.

This looks fine to me, but I would keep the old name --two-phase as
well. You could mark it as deprecated. No need to make a hard break.

On 22.06.25 15:38, Peter Eisentraut wrote:

On 17.06.25 20:19, Masahiko Sawada wrote:

Ideally, we should change both to maintain consistency across various
slot options. OTOH, as we have already described these options as: "
The --two-phase and --failover options can be specified with
--create-slot.", it is clear that these are slot options. The previous
version docs have a description: "The --two-phase can be specified
with --create-slot to enable decoding of prepared transactions." which
makes it even more clear that the two-phase is a slot option. The
options are named similarly in pg_create_logical_replication_slot API
and during CREATE SUBSCRIPTION, so, to some level, there is a
consistency in naming of these options across all APIs/tools. But,
their usage in a tool like pg_recvlogical could be perceived
differently as well, so it is also okay to consider naming them
differently.

Also note that we have a new pg_createsubscriber --enable-two-phase.

Yeah, I also noticed the precedent.

It would be nice if there was more consistency between similar/related
tools.

I've attached the patch. Feedback is very welcome.

This looks fine to me, but I would keep the old name --two-phase as
well.  You could mark it as deprecated.  No need to make a hard break.

I have committed your patch with this change.

On Mon, Jun 30, 2025 at 2:37 AM Peter Eisentraut <peter@eisentraut.org> wrote:

On 22.06.25 15:38, Peter Eisentraut wrote:

On 17.06.25 20:19, Masahiko Sawada wrote:

Ideally, we should change both to maintain consistency across various
slot options. OTOH, as we have already described these options as: "
The --two-phase and --failover options can be specified with
--create-slot.", it is clear that these are slot options. The previous
version docs have a description: "The --two-phase can be specified
with --create-slot to enable decoding of prepared transactions." which
makes it even more clear that the two-phase is a slot option. The
options are named similarly in pg_create_logical_replication_slot API
and during CREATE SUBSCRIPTION, so, to some level, there is a
consistency in naming of these options across all APIs/tools. But,
their usage in a tool like pg_recvlogical could be perceived
differently as well, so it is also okay to consider naming them
differently.

Also note that we have a new pg_createsubscriber --enable-two-phase.

Yeah, I also noticed the precedent.

It would be nice if there was more consistency between similar/related
tools.

I've attached the patch. Feedback is very welcome.

This looks fine to me, but I would keep the old name --two-phase as
well. You could mark it as deprecated. No need to make a hard break.

I have committed your patch with this change.

Thank you for taking over the patch! I was unable to work on this last
week due to other work.

Regards,

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