Horton

Thread

Commits

  1. Rename logical_replication_mode to debug_logical_replication_streaming

  1. logical_replication_mode

    Peter Eisentraut <peter@eisentraut.org> — 2023-08-24T06:14:44Z

    I suggest we rename this setting to something starting with debug_. 
Right now, the name looks much too tempting for users to fiddle with.  I 
think this is similar to force_parallel_mode.

Also, the descriptions in guc_tables.c could be improved.  For example,

     gettext_noop("Controls when to replicate or apply each change."),

is pretty content-free and unhelpful.

  2. Re: logical_replication_mode

    Amit Kapila <amit.kapila16@gmail.com> — 2023-08-25T04:28:13Z

    On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> I suggest we rename this setting to something starting with debug_.
> Right now, the name looks much too tempting for users to fiddle with.  I
> think this is similar to force_parallel_mode.
>

+1. How about debug_logical_replication?

> Also, the descriptions in guc_tables.c could be improved.  For example,
>
>      gettext_noop("Controls when to replicate or apply each change."),
>
> is pretty content-free and unhelpful.
>

The other possibility I could think of is to change short_desc as:
"Allows to replicate each change for large transactions.". Do you have
any better ideas?

-- 
With Regards,
Amit Kapila.

  3. RE: logical_replication_mode

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2023-08-25T06:52:14Z

    On Friday, August 25, 2023 12:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut <peter@eisentraut.org>
> wrote:
> >
> > I suggest we rename this setting to something starting with debug_.
> > Right now, the name looks much too tempting for users to fiddle with.
> > I think this is similar to force_parallel_mode.
> >
> 
> +1. How about debug_logical_replication?
> 
> > Also, the descriptions in guc_tables.c could be improved.  For
> > example,
> >
> >      gettext_noop("Controls when to replicate or apply each change."),
> >
> > is pretty content-free and unhelpful.
> >
> 
> The other possibility I could think of is to change short_desc as:
> "Allows to replicate each change for large transactions.". Do you have any
> better ideas?

How about "Forces immediate streaming or serialization of changes in large
transactions." which is similar to the description in document.

I agree that renaming it to debug_xx would be better and
here is a patch that tries to do this.

Best Regards,
Hou zj

  4. Re: logical_replication_mode

    Peter Eisentraut <peter@eisentraut.org> — 2023-08-25T07:08:45Z

    On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:
> On Friday, August 25, 2023 12:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut <peter@eisentraut.org>
>> wrote:
>>>
>>> I suggest we rename this setting to something starting with debug_.
>>> Right now, the name looks much too tempting for users to fiddle with.
>>> I think this is similar to force_parallel_mode.
>>>
>>
>> +1. How about debug_logical_replication?
>>
>>> Also, the descriptions in guc_tables.c could be improved.  For
>>> example,
>>>
>>>       gettext_noop("Controls when to replicate or apply each change."),
>>>
>>> is pretty content-free and unhelpful.
>>>
>>
>> The other possibility I could think of is to change short_desc as:
>> "Allows to replicate each change for large transactions.". Do you have any
>> better ideas?
> 
> How about "Forces immediate streaming or serialization of changes in large
> transactions." which is similar to the description in document.
> 
> I agree that renaming it to debug_xx would be better and
> here is a patch that tries to do this.

Maybe debug_logical_replication is too general?  Something like 
debug_logical_replication_streaming would be more concrete.  (Or 
debug_logical_streaming.)  Is that an appropriate name for what it's doing?

  5. Re: logical_replication_mode

    Amit Kapila <amit.kapila16@gmail.com> — 2023-08-25T09:55:46Z

    On Fri, Aug 25, 2023 at 12:38 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:
> > On Friday, August 25, 2023 12:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >>
> >> On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut <peter@eisentraut.org>
> >> wrote:
> >>>
> >>> I suggest we rename this setting to something starting with debug_.
> >>> Right now, the name looks much too tempting for users to fiddle with.
> >>> I think this is similar to force_parallel_mode.
> >>>
> >>
> >> +1. How about debug_logical_replication?
> >>
> >>> Also, the descriptions in guc_tables.c could be improved.  For
> >>> example,
> >>>
> >>>       gettext_noop("Controls when to replicate or apply each change."),
> >>>
> >>> is pretty content-free and unhelpful.
> >>>
> >>
> >> The other possibility I could think of is to change short_desc as:
> >> "Allows to replicate each change for large transactions.". Do you have any
> >> better ideas?
> >
> > How about "Forces immediate streaming or serialization of changes in large
> > transactions." which is similar to the description in document.
> >
> > I agree that renaming it to debug_xx would be better and
> > here is a patch that tries to do this.
>
> Maybe debug_logical_replication is too general?  Something like
> debug_logical_replication_streaming would be more concrete.
>

Yeah, that sounds better.

>  (Or
> debug_logical_streaming.)  Is that an appropriate name for what it's doing?
>

Yes.

-- 
With Regards,
Amit Kapila.

  6. RE: logical_replication_mode

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2023-08-27T12:05:40Z

    On Friday, August 25, 2023 5:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Fri, Aug 25, 2023 at 12:38 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> >
> > On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:
> > > On Friday, August 25, 2023 12:28 PM Amit Kapila
> <amit.kapila16@gmail.com> wrote:
> > >>
> > >> On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut
> > >> <peter@eisentraut.org>
> > >> wrote:
> > >>>
> > >>> I suggest we rename this setting to something starting with debug_.
> > >>> Right now, the name looks much too tempting for users to fiddle with.
> > >>> I think this is similar to force_parallel_mode.
> > >>>
> > >>
> > >> +1. How about debug_logical_replication?
> > >>
> > >>> Also, the descriptions in guc_tables.c could be improved.  For
> > >>> example,
> > >>>
> > >>>       gettext_noop("Controls when to replicate or apply each
> > >>> change."),
> > >>>
> > >>> is pretty content-free and unhelpful.
> > >>>
> > >>
> > >> The other possibility I could think of is to change short_desc as:
> > >> "Allows to replicate each change for large transactions.". Do you
> > >> have any better ideas?
> > >
> > > How about "Forces immediate streaming or serialization of changes in
> > > large transactions." which is similar to the description in document.
> > >
> > > I agree that renaming it to debug_xx would be better and here is a
> > > patch that tries to do this.
> >
> > Maybe debug_logical_replication is too general?  Something like
> > debug_logical_replication_streaming would be more concrete.
> >
> 
> Yeah, that sounds better.

OK, here is the debug_logical_replication_streaming version.

Best Regards,
Hou zj

  7. Re: logical_replication_mode

    Peter Smith <smithpb2250@gmail.com> — 2023-08-29T07:25:40Z

    Hi Hou-san.

I had a look at the patch 0001.

It looks OK to me, but here are a couple of comments:

======

1. Is this fix intended for PG16?

I found some mention of this GUC old name lurking in the release v16 notes [1].

~~~

2. DebugLogicalRepStreamingMode

-/* possible values for logical_replication_mode */
+/* possible values for debug_logical_replication_streaming */
 typedef enum
 {
- LOGICAL_REP_MODE_BUFFERED,
- LOGICAL_REP_MODE_IMMEDIATE
-} LogicalRepMode;
+ DEBUG_LOGICAL_REP_STREAMING_BUFFERED,
+ DEBUG_LOGICAL_REP_STREAMING_IMMEDIATE
+} DebugLogicalRepStreamingMode;

Shouldn't this typedef name be included in the typedef.list file?

------
[1] https://www.postgresql.org/docs/16/release-16.html

Kind Regards,
Peter Smith.
Fujitsu Australia

  8. Re: logical_replication_mode

    Amit Kapila <amit.kapila16@gmail.com> — 2023-08-29T08:21:27Z

    On Tue, Aug 29, 2023 at 12:56 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> I had a look at the patch 0001.
>
> It looks OK to me, but here are a couple of comments:
>
> ======
>
> 1. Is this fix intended for PG16?
>

Yes.

> I found some mention of this GUC old name lurking in the release v16 notes [1].
>

That should be changed as well but we can do that as a separate patch
just for v16.

-- 
With Regards,
Amit Kapila.

  9. RE: logical_replication_mode

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2023-08-29T09:44:54Z

    On Tuesday, August 29, 2023 3:26 PM Peter Smith <smithpb2250@gmail.com> wrote:

Thanks for reviewing.

> 2. DebugLogicalRepStreamingMode
> 
> -/* possible values for logical_replication_mode */
> +/* possible values for debug_logical_replication_streaming */
>  typedef enum
>  {
> - LOGICAL_REP_MODE_BUFFERED,
> - LOGICAL_REP_MODE_IMMEDIATE
> -} LogicalRepMode;
> + DEBUG_LOGICAL_REP_STREAMING_BUFFERED,
> + DEBUG_LOGICAL_REP_STREAMING_IMMEDIATE
> +} DebugLogicalRepStreamingMode;
> 
> Shouldn't this typedef name be included in the typedef.list file?

I think it's unnecessary to add this as there currently is no reference to the name.
See other similar examples like DebugParallelMode, RecoveryPrefetchValue ...
And the name is also not included in BF[1]

[1] https://buildfarm.postgresql.org/cgi-bin/typedefs.pl?branch=HEAD

Best Regards,
Hou zj

  10. Re: logical_replication_mode

    Peter Eisentraut <peter@eisentraut.org> — 2023-08-29T13:38:31Z

    On 27.08.23 14:05, Zhijie Hou (Fujitsu) wrote:
> On Friday, August 25, 2023 5:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Fri, Aug 25, 2023 at 12:38 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>>>
>>> On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:
>>>> On Friday, August 25, 2023 12:28 PM Amit Kapila
>> <amit.kapila16@gmail.com> wrote:
>>>>>
>>>>> On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut
>>>>> <peter@eisentraut.org>
>>>>> wrote:
>>>>>>
>>>>>> I suggest we rename this setting to something starting with debug_.
>>>>>> Right now, the name looks much too tempting for users to fiddle with.
>>>>>> I think this is similar to force_parallel_mode.
>>>>>>
>>>>>
>>>>> +1. How about debug_logical_replication?
>>>>>
>>>>>> Also, the descriptions in guc_tables.c could be improved.  For
>>>>>> example,
>>>>>>
>>>>>>        gettext_noop("Controls when to replicate or apply each
>>>>>> change."),
>>>>>>
>>>>>> is pretty content-free and unhelpful.
>>>>>>
>>>>>
>>>>> The other possibility I could think of is to change short_desc as:
>>>>> "Allows to replicate each change for large transactions.". Do you
>>>>> have any better ideas?
>>>>
>>>> How about "Forces immediate streaming or serialization of changes in
>>>> large transactions." which is similar to the description in document.
>>>>
>>>> I agree that renaming it to debug_xx would be better and here is a
>>>> patch that tries to do this.
>>>
>>> Maybe debug_logical_replication is too general?  Something like
>>> debug_logical_replication_streaming would be more concrete.
>>>
>>
>> Yeah, that sounds better.
> 
> OK, here is the debug_logical_replication_streaming version.

committed