Обсуждение: pg_recvlogical cannot create slots with failover=true

Dear hackers,

While reviewing threads I found $SUBJECT.

I think it does not affect to output, but I could not find strong reasons not to
support it. Also, this can be used for testing purpose, i.e., DBAs can verify the
slot-sync can work on their system by only using pg_recvlogical.

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

How do you feel?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

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